Skip to content

[LIVY-717] introduce maven property to set ZooKeeper version#262

Closed
symat wants to merge 1 commit intoapache:masterfrom
symat:LIVY-171
Closed

[LIVY-717] introduce maven property to set ZooKeeper version#262
symat wants to merge 1 commit intoapache:masterfrom
symat:LIVY-171

Conversation

@symat
Copy link
Copy Markdown
Contributor

@symat symat commented Nov 22, 2019

When we want to use Livy in a cluster where a newer ZooKeeper server version is used, we might run into run-time errors if we compile Livy using the current Curator / Hadoop versions. The Curator version can already explicitly set with the curator.version maven property in build time, but we were still missed the same parameter for ZooKeeper.

In this PR I added a new maven parameter called zookeeper.version and after analyzing the maven dependency tree, I made sure that the Curator and ZooKeeper versions used in compile time are always harmonized and controlled by the maven parameters.

I set the zookeeper.version in maven to 3.4.6 to be backward compatible with the current Livy dependencies.

see https://issues.apache.org/jira/browse/LIVY-717

@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 22, 2019

Codecov Report

Merging #262 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #262      +/-   ##
============================================
- Coverage     67.95%   67.88%   -0.07%     
+ Complexity      940      939       -1     
============================================
  Files           102      102              
  Lines          5876     5876              
  Branches        891      891              
============================================
- Hits           3993     3989       -4     
- Misses         1311     1315       +4     
  Partials        572      572
Impacted Files Coverage Δ Complexity Δ
...c/main/scala/org/apache/livy/repl/ReplDriver.scala 30.76% <0%> (-2.57%) 7% <0%> (ø)
...ain/scala/org/apache/livy/utils/SparkYarnApp.scala 66.01% <0%> (-1.31%) 40% <0%> (ø)
...ain/java/org/apache/livy/rsc/driver/RSCDriver.java 79.91% <0%> (-0.84%) 45% <0%> (ø)
...main/scala/org/apache/livy/server/LivyServer.scala 32.87% <0%> (+0.45%) 11% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f1e898...634d56b. Read the comment docs.

@jerryshao
Copy link
Copy Markdown
Contributor

Can it really be worked if you're using different version of ZK/Curator? I'm OK to make the version property configurable, but I'm more concerned about the question "can it really be worked after version is changed".

CC @yiheng I remembered you fixed a version incompatible issue in Curator, right?

@yiheng
Copy link
Copy Markdown
Contributor

yiheng commented Nov 25, 2019

Yes, here's the patch. The problem I have fixed is the curator-recipes lib version is not compatible with curator-client and curator-framework.

@yiheng
Copy link
Copy Markdown
Contributor

yiheng commented Nov 25, 2019

@jerryshao I think the current problem is even if we set the curator.version properly, the zk version from the dependency of curator maybe not what we really want.

@symat
Copy link
Copy Markdown
Contributor Author

symat commented Nov 25, 2019

Thanks for the comments!

even if we set the curator.version properly, the zk version from the dependency of curator maybe not what we really want.

yes, that is true as well. The new Curator 4.2.0 pulls ZK 3.5.4-beta, and we don't want to use a beta version in production. (the last stable official ZK release is 3.5.6)
The other problem is, that ZK comes not only from curator, but also from hadoop, so if we want to be sure that we only have a single ZooKeeper version on our classpath, then the best is to exclude it from both Curator and Hadoop and load our own version.

can it really be worked after version is changed

it depends on the combination of versions. You are right, that it won't work if you only change ZooKeeper version. Curator 2.x is only compatible with ZooKeeper 3.4.x, while Curator 3.x is compatible only with ZooKeeper 3.5.x. The new Curator 4.x is said to be compatible with both ZooKeeper versions, but we found during testing, that it is not necessarily works with the old ZooKeeper. (also one thing is compile time compatibility and an other the run-time one) So we found, that the best is to use the latest Curator only with the new ZooKeeper. (it is good to know, that on the Hadoop trunk we already have Curator 4.2.0 and ZooKeeper 3.5.6, but I am not aware of any released Hadoop version yet that contains this change)

So the trick is to make sure to use proper (compatible) Curator-Hadoop-ZooKeeper versions.

  • E.g. the current combination used by Livy: Curator 2.7.1, ZooKeeper 3.4.6, Hadoop 2.7.3
  • We are testing with the following versions: Curator 4.2.0, ZooKeeper 3.5.6, Hadoop 3.0.2

When you try to build Livy with the new Curator/ZooKeeper, then you will face an other small issue (one mock object needs to be changed in the integration tests), but other than that, Livy is working for us. Assuming you exclude the old ZooKeeper that comes from Hadoop.

I am mainly a ZooKeeper contributor, so I don't want to change the default Curator / ZooKeeper version in Livy, I think that is something the Livy community should decide / schedule. But I think it is an important feature to have the ability to explicitly set the ZooKeeper and Curator versions in Livy. So at least the community can test / experiment with the new third party versions.

Copy link
Copy Markdown
Contributor

@jerryshao jerryshao left a comment

Choose a reason for hiding this comment

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

LGTM.

@jerryshao jerryshao closed this in 0a527e8 Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants