Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JiriOndrusek
Copy link

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

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

@anmolnar
Copy link
Contributor

retest this please

@eolivelli
Copy link
Contributor

Please update maven pom as well

@JiriOndrusek
Copy link
Author

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?

@eolivelli
Copy link
Contributor

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

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

@eolivelli
Copy link
Contributor

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
Copy link
Contributor

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

@JiriOndrusek
Copy link
Author

@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")

@nkalmar
Copy link
Contributor

nkalmar commented May 14, 2019

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.

@JiriOndrusek
Copy link
Author

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.

@eolivelli
Copy link
Contributor

@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

@nkalmar
Copy link
Contributor

nkalmar commented May 15, 2019

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!

@JiriOndrusek JiriOndrusek force-pushed the ZOOKEEPER-3389_export-packages-osgi-master branch from 316962a to cf525f6 Compare May 15, 2019 13:55
@JiriOndrusek
Copy link
Author

JiriOndrusek commented May 15, 2019

@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?

@nkalmar
Copy link
Contributor

nkalmar commented May 15, 2019

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
Copy link
Contributor

nkalmar commented May 15, 2019

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.

@eolivelli
Copy link
Contributor

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

@JiriOndrusek JiriOndrusek force-pushed the ZOOKEEPER-3389_export-packages-osgi-master branch from cf525f6 to 60ef802 Compare May 15, 2019 15:16
@JiriOndrusek
Copy link
Author

@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 JiriOndrusek force-pushed the ZOOKEEPER-3389_export-packages-osgi-master branch from 60ef802 to bc59148 Compare May 15, 2019 15:34
@eolivelli
Copy link
Contributor

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

@JiriOndrusek
Copy link
Author

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.

@eolivelli
Copy link
Contributor

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

@JiriOndrusek
Copy link
Author

JiriOndrusek commented May 16, 2019

@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 JiriOndrusek force-pushed the ZOOKEEPER-3389_export-packages-osgi-master branch from bc59148 to affb736 Compare May 16, 2019 08:09
@nkalmar
Copy link
Contributor

nkalmar commented May 16, 2019

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!

@JiriOndrusek
Copy link
Author

@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 JiriOndrusek force-pushed the ZOOKEEPER-3389_export-packages-osgi-master branch from affb736 to c67506e Compare May 16, 2019 08:50
@JiriOndrusek
Copy link
Author

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
Copy link
Author

JiriOndrusek commented May 16, 2019

@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?

@nkalmar
Copy link
Contributor

nkalmar commented May 16, 2019

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.

@JiriOndrusek
Copy link
Author

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

@JiriOndrusek
Copy link
Author

JiriOndrusek commented May 16, 2019

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] #945

@JiriOndrusek
Copy link
Author

@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

Copy link
Contributor

@nkalmar nkalmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, tarballs unchanged, artifacts looks good.

@nkalmar
Copy link
Contributor

nkalmar commented May 30, 2019

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

@grgrzybek
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants