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

Issue #2106: Update ZookKeeper dependency to 3.5.7 #2112

Merged
merged 14 commits into from
May 7, 2020

Conversation

dmercuriali
Copy link
Contributor

@dmercuriali dmercuriali commented Jun 13, 2019

Descriptions of the changes in this PR:
Updated ZooKeeper dependency from 3.4.13 to 3.5.7
Excluded transitive dependency to netty-all.
Updated maven-shade-pluging configuration in 'distributedlog-core-shaded' project. Package 'org.apache.jute' was moved to a ZooKeeper dependency (org.apache.zookeeper:zookeeper-jute), so i added the artifact to the artifactSet.

Updated license files:

  • updated Zookeeper to 3.5.7
  • removed Jline 2.11
  • removed Netty 3.10.1
  • added Zookeeper-jute 3.5.7

Master Issue: #2106


  • [skip bookkeeper-server bookie tests]: skip testing org.apache.bookkeeper.bookie in bookkeeper-server module.
  • [skip bookkeeper-server client tests]: skip testing org.apache.bookkeeper.client in bookkeeper-server module.
  • [skip bookkeeper-server replication tests]: skip testing org.apache.bookkeeper.replication in bookkeeper-server module.
  • [skip bookkeeper-server tls tests]: skip testing org.apache.bookkeeper.tls in bookkeeper-server module.
  • [skip bookkeeper-server remaining tests]: skip testing all other tests in bookkeeper-server module.
  • [skip integration tests]: skip docker based integration tests. if you make java code changes, you shouldn't skip integration tests.
  • [skip build java8]: skip build on java8. ONLY skip this when ONLY changing files under documentation under site.
  • [skip build java11]: skip build on java11. ONLY skip this when ONLY changing files under documentation under site.

@sijie
Copy link
Member

sijie commented Jun 14, 2019

@dmercuriali thank you for working on this.

I think you removed one dependency from LICENSE file. Can you please check the travis CI to make sure it pass travis CI?

io.netty-netty-3.10.1.Final.jar unaccounted for in LICENSE



It looks like there are issues with the LICENSE/NOTICE.

See http://bookkeeper.apache.org/community/licensing for details on how to fix.

@eolivelli
Copy link
Contributor

Zookeeper 3.5.5 uses netty 4.
No need to have netty 3 anymore

@eolivelli
Copy link
Contributor

Maybe it is ending up in the binary tarball for some direct reference in some pom

@sijie
Copy link
Member

sijie commented Jun 14, 2019

@eolivelli I believe some other dependencies are still using netty 3

@eolivelli
Copy link
Contributor

@dmercuriali please check @sijie's comment.
Netty 3 is dead and it does not work well with java11 (it does close sockets properly).
This is the reason we moved zk 3.5 to netty 4 even if it was a release (non master) branch

@dmercuriali
Copy link
Contributor Author

@sijie you are right, Netty 3 is still required by

  • com.twitter:finagle-core_2.11 v6.44.0 (in BookKeeper stats providers :: Twitter finagle stats)
  • com.twitter:twitter-server_2.11 v1.29.0 (BookKeeper http :: Twitter http server), which has com.twitter:finagle-core_2.11 v6.44.0 as a dependency

In twitter-server_2.11 v1.31.0 / finagle-core_2.11 v7.0.0, Netty 3 dependency was moved to test scope.

I'll restore Netty 3 license entries.

@eolivelli
Copy link
Contributor

eolivelli commented Jun 17, 2019

run bookkeeper-server bookie tests

@eolivelli
Copy link
Contributor

run integration tests

5 similar comments
@dmercuriali
Copy link
Contributor Author

run integration tests

@eolivelli
Copy link
Contributor

run integration tests

@eolivelli
Copy link
Contributor

run integration tests

@dmercuriali
Copy link
Contributor Author

run integration tests

@dmercuriali
Copy link
Contributor Author

run integration tests

@dmercuriali
Copy link
Contributor Author

Integration tests keep failing. The failing test is not always the same, but most of the time it's 'org.apache.bookkeeper.tests.backwardcompat.TestCompatUpgrade' or a cluster test.

I'm able to run backward compatibility tests locally with success all the time.
The cluster test fails with a 'Could not initialize container' error (looks like it's waiting for ZooKeeper to start)

@eolivelli
Copy link
Contributor

@ivankelly @sijie
It seems a problem of the asf infrastructure
Most of the times the problem is about starting the docker container

@eolivelli
Copy link
Contributor

run integration tests

@Ghatage
Copy link
Contributor

Ghatage commented Jun 28, 2019

@eolivelli Do we want to again use the newer 3.5.x ZK API's which were reverted as a part of #1601 ?

@eolivelli
Copy link
Contributor

@Ghatage feel free to send a follow up patch.

Currently we have a little problem with this one

@eolivelli
Copy link
Contributor

run integration tests

@eolivelli
Copy link
Contributor

eolivelli commented Jun 29, 2019

@dmercuriali are you sure that tests are running fine on your laptop?

@eolivelli
Copy link
Contributor

Please take a look to #1601
There were fixes about integration tests

@jvrao
Copy link
Contributor

jvrao commented Jun 29, 2019

@eolivelli Do we want to again use the newer 3.5.x ZK API's which were reverted as a part of #1601 ?

@sijie @merlimat can you please comment ?

@sijie
Copy link
Member

sijie commented Jun 30, 2019

I think we should try to use the newer version of zookeeper API since zookeeper has provided the 3.5 stable release. To be honest, 3.5 has been used in large internet companies for a long time, so they are quite stable to use.

@dmercuriali
Copy link
Contributor Author

Sorry for the delay
@eolivelli yes, tests are fine on my end. Just ran them several more times and both 'backward compatibility' and 'cluster' tests passed all times.

@eolivelli
Copy link
Contributor

run integration tests

@eolivelli
Copy link
Contributor

@dmercuriali I guess you have to take a look to #1601 and at least back port the changes to the integration tests

@eolivelli
Copy link
Contributor

run integration tests

1 similar comment
@eolivelli
Copy link
Contributor

run integration tests

@eolivelli
Copy link
Contributor

@dmercuriali it seems that you now have a single test failing constantly on CI
org.apache.bookkeeper.tests.integration.cluster.SimpleClusterTest.test001_StartBookie

@eolivelli
Copy link
Contributor

@dmercuriali it looks like even my clone of this patch is stuck in the same jobs
what a nuisance !
there must be something weird with this patch

@eolivelli
Copy link
Contributor

Locally the build is stuck here for me, with MVN 3.6.3 and JDK8

INFO] --- maven-source-plugin:2.2.1:jar (attach-sources) @ distributedlog-core-shaded ---
[INFO] Building jar: /Users/enrico.olivelli/dev/bookkeeper/shaded/distributedlog-core-shaded/target/distributedlog-core-shaded-4.11.0-SNAPSHOT-sources.jar
[INFO] 
[INFO] --- maven-shade-plugin:3.2.0:shade (default) @ distributedlog-core-shaded ---
[INFO] Including org.apache.distributedlog:distributedlog-core:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including org.apache.distributedlog:distributedlog-protocol:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including org.apache.distributedlog:distributedlog-common:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including org.apache.bookkeeper.stats:bookkeeper-stats-api:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including com.google.guava:guava:jar:21.0 in the shaded jar.
[INFO] Including net.jpountz.lz4:lz4:jar:1.3.0 in the shaded jar.
[INFO] Including org.apache.zookeeper:zookeeper:jar:3.5.7 in the shaded jar.
[INFO] Including org.apache.zookeeper:zookeeper-jute:jar:3.5.7 in the shaded jar.
[INFO] Excluding org.apache.yetus:audience-annotations:jar:0.5.0 from the shaded jar.
[INFO] Excluding io.netty:netty-handler:jar:4.1.32.Final from the shaded jar.
[INFO] Excluding io.netty:netty-transport:jar:4.1.32.Final from the shaded jar.
[INFO] Excluding io.netty:netty-resolver:jar:4.1.32.Final from the shaded jar.
[INFO] Excluding io.netty:netty-codec:jar:4.1.32.Final from the shaded jar.
[INFO] Excluding io.netty:netty-transport-native-epoll:jar:4.1.32.Final from the shaded jar.
[INFO] Excluding io.netty:netty-transport-native-unix-common:jar:4.1.32.Final from the shaded jar.
[INFO] Including org.apache.bookkeeper:bookkeeper-server:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including org.apache.bookkeeper:bookkeeper-common:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including org.apache.bookkeeper:cpu-affinity:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including com.fasterxml.jackson.core:jackson-core:jar:2.9.7 in the shaded jar.
[INFO] Including com.fasterxml.jackson.core:jackson-databind:jar:2.9.7 in the shaded jar.
[INFO] Including com.fasterxml.jackson.core:jackson-annotations:jar:2.9.7 in the shaded jar.
[INFO] Excluding org.jctools:jctools-core:jar:2.1.2 from the shaded jar.
[INFO] Including org.apache.bookkeeper:bookkeeper-common-allocator:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including org.apache.bookkeeper:bookkeeper-proto:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including com.google.protobuf:protobuf-java:jar:3.5.1 in the shaded jar.
[INFO] Including org.apache.bookkeeper:bookkeeper-tools-framework:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including org.rocksdb:rocksdbjni:jar:5.13.1 in the shaded jar.
[INFO] Excluding io.netty:netty-transport-native-epoll:jar:linux-x86_64:4.1.32.Final from the shaded jar.
[INFO] Excluding io.netty:netty-tcnative-boringssl-static:jar:2.0.20.Final from the shaded jar.
[INFO] Including org.apache.bookkeeper.http:http-server:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including org.apache.bookkeeper:circe-checksum:jar:4.11.0-SNAPSHOT in the shaded jar.
[INFO] Including commons-cli:commons-cli:jar:1.2 in the shaded jar.
[INFO] Including commons-codec:commons-codec:jar:1.6 in the shaded jar.
[INFO] Including commons-io:commons-io:jar:2.4 in the shaded jar.
[INFO] Including org.apache.commons:commons-lang3:jar:3.6 in the shaded jar.
[INFO] Including org.apache.commons:commons-collections4:jar:4.1 in the shaded jar.
[INFO] Excluding org.bouncycastle:bcpkix-jdk15on:jar:1.60 from the shaded jar.
[INFO] Excluding org.bouncycastle:bcprov-jdk15on:jar:1.60 from the shaded jar.
[INFO] Excluding org.bouncycastle:bcprov-ext-jdk15on:jar:1.60 from the shaded jar.
[INFO] Excluding com.beust:jcommander:jar:1.48 from the shaded jar.
[INFO] Including net.java.dev.jna:jna:jar:3.2.7 in the shaded jar.
[INFO] Excluding org.slf4j:slf4j-api:jar:1.7.25 from the shaded jar.
[INFO] Excluding commons-configuration:commons-configuration:jar:1.10 from the shaded jar.
[INFO] Including commons-lang:commons-lang:jar:2.6 in the shaded jar.
[INFO] Including commons-logging:commons-logging:jar:1.1.1 in the shaded jar.
[WARNING] distributedlog-core-4.11.0-SNAPSHOT.jar, bookkeeper-server-4.11.0-SNAPSHOT.jar define 1 overlapping classes: 
[WARNING]   - org.apache.bookkeeper.client.package-info
[WARNING] bookkeeper-stats-api-4.11.0-SNAPSHOT.jar, bookkeeper-server-4.11.0-SNAPSHOT.jar define 1 overlapping classes: 
[WARNING]   - org.apache.bookkeeper.stats.package-info
[WARNING] distributedlog-protocol-4.11.0-SNAPSHOT.jar, distributedlog-core-4.11.0-SNAPSHOT.jar define 2 overlapping classes: 
[WARNING]   - org.apache.distributedlog.exceptions.package-info
[WARNING]   - org.apache.distributedlog.package-info
[WARNING] bookkeeper-common-4.11.0-SNAPSHOT.jar, bookkeeper-server-4.11.0-SNAPSHOT.jar define 1 overlapping classes: 
[WARNING]   - org.apache.bookkeeper.util.package-info
[WARNING] maven-shade-plugin has detected that some class files are
[WARNING] present in two or more JARs. When this happens, only one
[WARNING] single version of the class is copied to the uber jar.
[WARNING] Usually this is not harmful and you can skip these warnings,
[WARNING] otherwise try to manually exclude artifacts based on
[WARNING] mvn dependency:tree -Ddetail=true and the above output.
[WARNING] See http://maven.apache.org/plugins/maven-shade-plugin/
[INFO] Replacing original artifact with shaded artifact.
[INFO] Replacing /Users/enrico.olivelli/dev/bookkeeper/shaded/distributedlog-core-shaded/target/distributedlog-core-shaded-4.11.0-SNAPSHOT.jar with /Users/enrico.olivelli/dev/bookkeeper/shaded/distributedlog-core-shaded/target/distributedlog-core-shaded-4.11.0-SNAPSHOT-shaded.jar
[INFO] Dependency-reduced POM written at: /Users/enrico.olivelli/dev/bookkeeper/shaded/distributedlog-core-shaded/dependency-reduced-pom.xml

@eolivelli
Copy link
Contributor

It looks like a problem with the maven shade plugin

main" #1 prio=5 os_prio=31 tid=0x00007fa723009000 nid=0x2203 runnable [0x00007000009ba000]
   java.lang.Thread.State: RUNNABLE
	at org.jdom2.Element.isAncestor(Element.java:1052)
	at org.jdom2.ContentList.checkPreConditions(ContentList.java:222)
	at org.jdom2.ContentList.add(ContentList.java:244)
	at org.jdom2.Element.addContent(Element.java:950)
	at org.apache.maven.plugins.shade.pom.MavenJDOMWriter.insertAtPreferredLocation(MavenJDOMWriter.java:292)
	at org.apache.maven.plugins.shade.pom.MavenJDOMWriter.iterateExclusion(MavenJDOMWriter.java:488)
	at org.apache.maven.plugins.shade.pom.MavenJDOMWriter.updateDependency(MavenJDOMWriter.java:1335)
	at org.apache.maven.plugins.shade.pom.MavenJDOMWriter.iterateDependency(MavenJDOMWriter.java:386)
	at org.apache.maven.plugins.shade.pom.MavenJDOMWriter.updateModel(MavenJDOMWriter.java:1623)
	at org.apache.maven.plugins.shade.pom.MavenJDOMWriter.write(MavenJDOMWriter.java:2156)
	at org.apache.maven.plugins.shade.pom.PomWriter.write(PomWriter.java:75)
	at org.apache.maven.plugins.shade.mojo.ShadeMojo.rewriteDependencyReducedPomIfWeHaveReduction(ShadeMojo.java:1049)
	at org.apache.maven.plugins.shade.mojo.ShadeMojo.createDependencyReducedPom(ShadeMojo.java:978)
	at org.apache.maven.plugins.shade.mojo.ShadeMojo.execute(ShadeMojo.java:538)
	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:137)

@eolivelli
Copy link
Contributor

If you run mvn with "debug"
mvn clean install spotbugs:check -DskipTests -rf :distributedlog-core-shaded -X

You get this loop:

DEBUG] updateExcludesInDeps()
[DEBUG] building maven31 dependency graph for org.apache.distributedlog:distributedlog-core-shaded:jar:4.11.0-SNAPSHOT with Maven31DependencyGraphBuilder
[DEBUG] Dependency collection stats: {ConflictMarker.analyzeTime=13578, ConflictMarker.markTime=7717, ConflictMarker.nodeCount=77, ConflictIdSorter.graphTime=10232, ConflictIdSorter.topsortTime=6263, ConflictIdSorter.conflictIdCount=39, ConflictIdSorter.conflictIdCycleCount=0, ConflictResolver.totalTime=108196, ConflictResolver.conflictItemCount=64, DefaultDependencyCollector.collectTime=235817, DefaultDependencyCollector.transformTime=152216}

I guess the problem is about the "exclusion" of netty-all or something like that
maybe you have to work con distributedlog-core-shaded module

pom.xml Outdated Show resolved Hide resolved
@dmercuriali dmercuriali requested a review from eolivelli May 6, 2020 15:19
@eolivelli
Copy link
Contributor

Hurray !
all tests passed
great work @dmercuriali

@mino181295 PTAL

@sijie @jiazhai @merlimat @rdhabalia Pulsar is already on ZK 3.5.
The same is for all of our products like HerdDB.
I wish to commit this patch to master, please take a look

@eolivelli eolivelli merged commit fe25574 into apache:master May 7, 2020
Ghatage pushed a commit to Ghatage/bookkeeper that referenced this pull request Jun 19, 2020
Descriptions of the changes in this PR:
Updated ZooKeeper dependency from 3.4.13 to 3.5.7
Excluded transitive dependency to netty-all.
Updated maven-shade-pluging configuration in 'distributedlog-core-shaded' project. Package 'org.apache.jute' was moved to a ZooKeeper dependency (org.apache.zookeeper:zookeeper-jute), so i added the artifact to the artifactSet.

Updated license files:
- updated Zookeeper to 3.5.7
- removed Jline 2.11
- removed Netty 3.10.1
- added Zookeeper-jute 3.5.7

Master Issue: apache#2106 



Reviewers: Matteo Minardi <minardi.matteo@hotmail.it>, Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <zhaijia@apache.org>

This closes apache#2112 from dmercuriali/fix/apache#2106-update-zookeeper, closes apache#2106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants