zookeeper icon indicating copy to clipboard operation
zookeeper copied to clipboard

[ZOOKEEPER-3389] Zookeeper does not export all required packages in OSGi (needed for curator)

Open JiriOndrusek opened this issue 5 years ago • 30 comments

Issue: https://issues.apache.org/jira/browse/ZOOKEEPER-3389

Added exported-packages which are required for Curator installation in OSGi.

JiriOndrusek avatar May 13 '19 10:05 JiriOndrusek

retest this please

anmolnar avatar May 13 '19 11:05 anmolnar

Please update maven pom as well

eolivelli avatar May 13 '19 11:05 eolivelli

Hi @eolivelli , I'd like to upgrade also pom, but I'm not able to find any plugin creating OSGi headers in mvn (mainly I'm looking into https://github.com/apache/zookeeper/blob/master/pom.xml) Can you please help me to find it?

JiriOndrusek avatar May 13 '19 12:05 JiriOndrusek

Let me check. Maybe we lost ot in the migration.

@anmolnar this will be a -1 on the 3.5.5rc6

eolivelli avatar May 13 '19 12:05 eolivelli

Is this @JiriOndrusek a "regression" ? As you are adding the manifest lines on ANT build I guess this is not a problem introduced by the recent Maven Migration

cc @nkalmar @anmolnar

eolivelli avatar May 14 '19 08:05 eolivelli

Can you please try to add a fix even for the maven build ?

eolivelli avatar May 14 '19 08:05 eolivelli

@eolivelli Hi, I've checked history of build.xml and it is not a regression. Just an issue, which could be seen only after installing Curator 4.1.1 (may be older) in OSGi.

For maven: I haven't found any occurrence of OSGi headers in main pom.xml. I'm not sure whether I just misses them -> so only a little fix would be needed or whether it has to be added -> slightly bigger change.

I can look into it later this week, is this time horizon acceptable? (I see in comment above "this will be a -1 on the 3.5.5rc6")

JiriOndrusek avatar May 14 '19 12:05 JiriOndrusek

Hi @JiriOndrusek , Thanks for this patch and looking into this. Unfortunately there is no maven OSGi whatsoever, so it needs to be added.

As this is not a regression, I believe rc6 will be rolled out, still short on binding +1 though. So at my current knowledge, target version for this is 3.5.6. But it needs a maven fix for sure. I'll also try to look into it, I'll let you know if I find anything.

nkalmar avatar May 14 '19 13:05 nkalmar

Hi @nkalmar , I've started with osgification of maven build, so far in my branch https://github.com/JiriOndrusek/zookeeper/commits/osgi_in_maven If you've already started with this task also, we can calaborate.

JiriOndrusek avatar May 15 '19 08:05 JiriOndrusek

@JiriOndrusek I feel you can continue on this branch, we can merge all this OSGI stuff in a single commit. On branch 3.5 and master we are not using Ant anymore. Ant is living mostly only for 3.4

eolivelli avatar May 15 '19 08:05 eolivelli

I agree with Enrico, please just continue on this branch, as maven will be used to release future 3.5.x and 3.6.x versions. I don't have much to add, but I'll keep a lookout for this PR!

nkalmar avatar May 15 '19 09:05 nkalmar

@nkalmar @eolivelli I'm getting close to the finishing. I just want to clarify some facts and also ask for the help.

  • just to be sure, only zookeeper-server jar will be compiled as OSGi bundle? (I see also possible modules jute and recipes, so I would like to be sure)
  • I'm currently facing a problem with OSGi build, there is a warning that zookeeper-server is exporting package, which is also present in different jar - zookeeper-jute. (Packages org/apache/zookeeper/server/quorum and org/apache/zookeeper/server/persistence) It would help me to know, how these packages should be handled. I looked into package 'quorum' and it seems, that it contains different classes. But even if they are different, it could change in the FUTURE. So I'd like to ask, how to face it
    • best solution from OSGi would be to change jute to use different package names - but it is probably not possible
    • best way (from my POV) is to merge packages together (with rule that classes from zookeeper-server has to be treated as primary) - but it can remove some classes which are needed to run and I have also problems to define it with maven-plugin (still not sure why it is not working, I think it should work)
    • may be it is possible also to ignore these packages from zookeeper-jute - but it doesn't seem much probable

What do you think about it?

JiriOndrusek avatar May 15 '19 14:05 JiriOndrusek

There will be 2 jars upon release, zookeeper-jute and zookeeper-server. zookeeper-jute is the serialization library (just like protobuf). zookeeper-server depends on zookeeper-jute. Since jute (originally) was an independent project, it was always kept seperate. But it evolved with zookeeper, hence the same package names. It's not ideal, and even causes problems like this one with OSGi.

The jute classes are generated, just like in protobuf. I'm afraid merging the packages together is not.. uhm, preferable, to say. The most plausible solution out of these for me seems to be changing jute package names. Those should be all private to ZooKeeper, so we do not change public API. But I would be happy if we found another solution.

Unfortunately I don't know much about OSGi, so I don't know what the best solution would be here. But zookeeper-server needs zookeeper-jute to serialize the messages between nodes. Maven automatically downloads zookeeper-jute when zookeeper-server is used, as it is a dependency for it. I'm not sure how OSGi solves this.

nkalmar avatar May 15 '19 14:05 nkalmar

I see you changed zookeeper-server packaging from jar to bundle. Yeah... I will check up on this sometime this week. I'm afraid this also affects other things.

nkalmar avatar May 15 '19 14:05 nkalmar

Yes changing the package name is feasible but only for 3.6+.

eolivelli avatar May 15 '19 14:05 eolivelli

@nkalmar @eolivelli I'll continue with this tomorrow. I see in your comment: "Those should be all private to ZooKeeper, so we do not change public API" and the fact that jute is another artifact, I see "simple" solution: I'll try to dig up little bit if there is some way, when zookeeper-jute won't export these "private packages". But I have to try some possibilities, how to solve zookeeper-server's dependency on those libraries. I'm not sure whether it will be possible. Current status is:

  • build changes are only for zookeeper-server to be changed into OSGi bundle,
  • there are no warnings during build.
  • I have to validate whether there is some "real" difference between current manifest and manifest generated by ant.

As there is a mentioning, that changing packaging from 'jar' to 'bundle' will cause some errors elsewhere. Unfortunately I'm not aware of other way of running OSGi build, but I'll try to look into it also tomorrow.

JiriOndrusek avatar May 15 '19 15:05 JiriOndrusek

Maybe you can simply add raw entries to the manifest in the jar plugin configuration

eolivelli avatar May 15 '19 15:05 eolivelli

Hi @nkalmar @eolivelli,

let me introduce "a better solution" for the problem with same package's names of zookeeper-jute and zookeeper-server.

If I understand it correctly, maven way of solving this problem is acceptable even if it causes problems sometimes. From previous comment - "It's not ideal, and even causes problems like this one with OSGi." Maven solution is that in case that packages DOESN'T contain the same classes - it works as EXPECTED. If it contains the same classes, then classloader return class from jar based on ORDER of jars. (so even if I want clas from jute, I can get class from server) Which is typicalli zookeeper-server and then zookeeper-jute (as jute is dependency from server)

I propose to use OSGi merge capability, which brings the same behavior. If someone wants class from that shared package, in case that classes are differen in both jars, this merge "feature" will behave as expected and correct class is returned. In case that there are overlapping classes, we define, which jar has advantage. (so I'd say that zookeeper-server has advantage). IMHO behavior will be a little bit better then in maven, because it will behave always in the same way. (In reality it won't robabnly happen, because the same classes in the shared packages will probably cause problems elsewhere, so everyone will try to avoid that)

In other words - behavior of OSGI with merge will be the same as maven if packages contains different classes. If in the future there will be the overlapping classes, behavior in OSGi would be defined (in contrast with maven, where it would ddepend on order of jar loading - so almost "random")

Possibly (I'm not sure about it in this time) I can also try to add some insurance/condition, that it will work for 3.5.x only. I mean that after upgrading to higher version of zookeeper, it will cause error during build (or at least warning), so someone will look into it and will find a comment explaining, that correct solution is to rename packages -> which is the correct solution. In case that packages won't be renamed at that time. This condition would allow us to solve problem in 3.5.x time, where packages couldn't be renamed.

Just to avoid confusion, this merge capability only defines, how OSGi returns classes if the class is in package, which is shared in more jars. No actuall merging of compiled classes is not happening.

JiriOndrusek avatar May 15 '19 19:05 JiriOndrusek

It would be great to have some integration test which loads zookeeper client with osgi and this way we will ensure that we are delivering something that really works. We can do this by adding another module to the build which uses the client inside an osgi container. It is not so simple (because the test will need to start at least one server and use the client and server and client are packaged inside of the same jars).

We have to setup these kind of integration tests (maybe docker based) in the short term, not necessarily for thia case. I will be happy to work on this.

Maybe you can continue with the osgi packaging stuff and I will then try to setup the integration test in a followup patch

eolivelli avatar May 15 '19 21:05 eolivelli

@eolivelli Ok, i'll continue with osgi packaging. I'll apply this also to zookeeper-jute and somehow solve "the same package name" problem in the way as described in my previous post.

Integration test validating that OSGi is working, would be really nice.

JiriOndrusek avatar May 16 '19 06:05 JiriOndrusek

What I meant in "private package" is that we do not expose any of this to clients. Jute is only used to serialize messages between ZooKeeper nodes, client does not use Jute for serialization. In theory, anyone could use zookeeper-jute for serialization, in reality, no one will and no one should. But it was agreed upon jute should be a different module still. And on a side note, some want to replace jute with a more recent and actively developed serialization library like protobuf or avro. But, for now, jute is here to stay, and it might only be replaced in a new major version (like 4.0.0).

Anyway, thanks again for looking into this, the merge stuff sounds good!

nkalmar avatar May 16 '19 08:05 nkalmar

@nkalmar I've added OSGi support to zookeeper-jute. I had to change pockaging to bundle as well, but in maven environment it behaves still as jar packaging. @eolivelli I think that it is possible to prepare integration test. I'll try to verify both bundles manually (that they could be deployed) I've talked to @grgrzybek and he could possibly help with integration test.

I think that current state is, that both jars (z-server and z-jute) are also osgi bundles. Now I plan to fine tuning dependencies during manual installation.

JiriOndrusek avatar May 16 '19 08:05 JiriOndrusek

I was able to install manually zookeeper with curator. But I had to change a little bit export-packages in zookeeper-server, which throws now warning during build about package "org.apache.zookeeper.data".

Warning message looks like: zookeeper:bundle:3.6.0-SNAPSHOT : Export org.apache.zookeeper, has 1, private references [org.apache.zookeeper.data] zookeeper:bundle:3.6.0-SNAPSHOT : Export org.apache.zookeeper.cli, has 1, private references [org.apache.zookeeper.data] zookeeper:bundle:3.6.0-SNAPSHOT : Export org.apache.zookeeper.client, has 1, private references [org.apache.zookeeper.data] zookeeper:bundle:3.6.0-SNAPSHOT : Export org.apache.zookeeper.server, has 1, private references [org.apache.zookeeper.data] zookeeper:bundle:3.6.0-SNAPSHOT : Export org.apache.zookeeper.server.auth, has 1, private references [org.apache.zookeeper.data] zookeeper:bundle:3.6.0-SNAPSHOT : Export org.apache.zookeeper.server.quorum, has 1, private references [org.apache.zookeeper.data] zookeeper:bundle:3.6.0-SNAPSHOT : Export org.apache.zookeeper.proto, has 1, private references [org.apache.zookeeper.data]

Meaning of this warning is, that some classes from zookeeper (zookeeper.cli, ...) for example returns classes from package "org.apache.zookeeper.data" which is not exported from the bundle. I have to solve this minor thing.

JiriOndrusek avatar May 16 '19 09:05 JiriOndrusek

@eolivelli @nkalmar I've found problem with current approach (2 bundles, one is server and second is jute) Problem:

  • zookeeper-jute has package 'org.apache.zookeeper.data', it has to be exported for zookeeper-server to use it
  • zookeeper-server contais also package 'org.apache.zookeeper.data', which has to be exported, because it is use from packages like org.apache.zookeeper -> bundles can not be deployed into karaf as two libraries are exporting the same packages

Solution:

  • best solution is to change name of one of these packages (probably in zookeeper-jute - which us used only by zookeeper) - but as @eolivelli wrote: "Yes changing the package name is feasible but only for 3.6+."
  • only other solution is to create only one bundle (e.g. zookeeper-osgi), which will contain both libraries together and will expose theit both packages at the same time (simillar approach is used in e.g. hibernate-osgi, httpcore-osgi, httpclient-osgi)

But with the fact that in the 3.6+ version renaming is possible so we can use solution #1 from 3.6+ as it is a better solution, i would like to ask on your opinion about current version. Is renaming of jute's packages really unreal. Is creation of zookeeper-osgi library the only solution?

JiriOndrusek avatar May 16 '19 10:05 JiriOndrusek

We would still need to solve it for 3.5 branch. So, for me, creating a zookeeper-osgi seems the way to go, but this should be voted on the zookeeper-dev mail list.

Can you write an email to the dev list @JiangJiafu with your findings?

A short summary, and then you can link this PR.

nkalmar avatar May 16 '19 10:05 nkalmar

@nkalmar I've just asked for subscription to dev mailing list, then I'd be able to send an email there.

JiriOndrusek avatar May 16 '19 10:05 JiriOndrusek

I've sent email to zookeeper-dev mailing list:

ZooKeeper and OSGi (maven)

Hi,

I 've created issue [1] with missing exported packages in osgi for zookeeper 3.4.10. Then I started to prepare maven OSGi packaging [2] for the higher version of ZooKeeper (in the PR for issue).

I've tried to implement OSGi packaging with the low impact. So I've tried to create OSGi bundles from Zookeeper-server and from zookeeper-jute modules.

But there is a problem for this solution:

zookeeper-jute has package 'org.apache.zookeeper.data', it has to be exported for zookeeper-server to use it
zookeeper-server contains also package 'org.apache.zookeeper.data', which has to be exported, because it is used from packages like org.apache.zookeeper, which are exported

-> bundles can not be deployed into osgi as two libraries are exporting the same packages

Solution:

best solution is to change name of one of these packages (probably in module zookeeper-jute - which us used only by zookeeper) - but question is, whether this change is feasible
only other solution is to create only one bundle (e.g. zookeeper-osgi), which will contain both libraries together and will expose their both packages at the same time (similar approach is used in e.g. hibernate-osgi, httpcore-osgi, httpclient-osgi)

Solution #1 is a better solution, I would like to ask for your opinion about feasibility of renaming zookeeper-jute generated packages to not collide with zookeeper-server. (As these packages are to be used only for zookeeper, it shouldn't cause any harm)

If #1 is not acceptable, then we can go with #2. But I highly suggest to consider renaming of zookeeper-jute's packages in the nearest point in the future as possible and return to solution #1.

Best regards,

jiri

[1] https://issues.apache.org/jira/browse/ZOOKEEPER-3389 [2] https://github.com/apache/zookeeper/pull/945

JiriOndrusek avatar May 16 '19 11:05 JiriOndrusek

@nkalmar Solution #2 seems to be feasible - I've just tried simple prototype in different branch: https://github.com/JiriOndrusek/zookeeper/commits/zookeeper-osgi-module

JiriOndrusek avatar May 16 '19 13:05 JiriOndrusek

Thanks @JiriOndrusek , I'll ping @eolivelli if he has anything to add, otherwise LGTM! I'll also check 3.5 fix...

nkalmar avatar May 30 '19 14:05 nkalmar

Excuse me for being late to the party.

Let me introduce myself - I'm OSGi dinosaur and I'd be happy to help here.

So zookeeper-jute jar contains (generated from zookeeper-jute/src/main/resources/zookeeper.jute) these packages:

  • org.apache.zookeeper.data
  • org.apache.zookeeper.proto
  • org.apache.zookeeper.server.quorum
  • org.apache.zookeeper.server.persistence
  • org.apache.zookeeper.txn

org.apache.zookeeper.data is fine there, but org.apache.zookeeper.server.quorum and org.apache.zookeeper.server.persistence are duplicated in zookeeper jar, thus simple OSGIfication of zookeeper + zookeeper-jute is not possible (can't have single package exported from two OSGi bundles. I mean it's possible, but may lead to unpredictable results - you can't tell which package your importing bundle will wire to).

I understand that some classes of org.apache.zookeeper.server.quorum are written and some are generated from zookeeper.jute and it's fine in flat classpath scenario.

For OSGi, you'd have to do what for example Hibernate or httpclient (before silently dropping OSGi support in httpclient 5) is doing - there should be zookeeper-osgi.jar which just embeds both zookeeper and zookeeper-jute. This is best approach, because there'll be only one Maven artifact with <packaging>bundle</packaging>.

I'll continue checking.

grgrzybek avatar Jun 03 '19 08:06 grgrzybek