HBASE-28153 Upgrade zookeeper to a newer version #5475
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ZooKeeper has changed their netty dependencies so we need to modify our netty dependencies and exclusions too. And ZooKeeper also specify some necessary dependencies for running ZooKeeperServer as provided, so we need to specify them directly in our pom as we have a MiniZooKeeperCluster, where we will run a ZooKeeperServer. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ndimiduk
approved these changes
Oct 25, 2023
hbase-common/pom.xml
Outdated
| @@ -97,6 +97,15 @@ | |||
| <groupId>io.opentelemetry</groupId> | |||
| <artifactId>opentelemetry-semconv</artifactId> | |||
| </dependency> | |||
| <!-- include netty dependencies as hadoop needs them--> | |||
There was a problem hiding this comment.
Is it possible to push these out to the server or test modules that make use of them? Or are we fiddling with zookeeper-server + netty classes from within hbase-common ?
There was a problem hiding this comment.
I agree i'd prefer to not have these in common if possible
There was a problem hiding this comment.
The dependencies for netty here are not about zookeeper, it is because hadoop needs them...
There was a problem hiding this comment.
Ah, shit, we only depends on hadoop-common in hbase-common, not hadoop-hdfs. Hadoop-common does not depend on netty directly but it depends on zookeeper, as while as curator...
Let me think how to only set necessary dependencies here...
There was a problem hiding this comment.
Shit I got 0e issue on my 980 Pro and my vmware disk files are corrupted...
I'm on a business trip so do not have ways to backup the files and fix the problem...
Will be back to Beijing tomorrow, please give me some moe time on this issue...
There was a problem hiding this comment.
I only include the netty4 dependency here to reflect that we depend on hadoop-common -> zookeeper, and the pre commit result seems fine. I'm not sure whether netty4 is only used by zookeeper server but it is declared as a compile scope dependency for zookeeper, so I prefer we add this dependency here.
|
I closed HBASE-28178 (#5479) as a dup of this but we need to upgrade ZooKeeper to at least 3.7.2 on all branches because of CVE-2023-44981. /cc @ndimiduk @bbeaudreault |
apurtell
approved these changes
Oct 26, 2023
| <!-- ZooKeeperServer needs the below dependencies, thus MiniZooKeeperCluster also needs them --> | ||
| <dependency> | ||
| <groupId>org.xerial.snappy</groupId> | ||
| <artifactId>snappy-java</artifactId> |
There was a problem hiding this comment.
This undeclared dependency of recent zookeeper versions is annoying. We will CNFE without it.
There was a problem hiding this comment.
Yes, I think a better way is to have a zookeeper-client module and a zookeeper-server module. Now they only have a zookeeper module but they only declare the dependencies for zookeeper-client at compile scope so we need to manually add the zookeeper server dependencies...
| </dependency> | ||
| <dependency> | ||
| <groupId>commons-cli</groupId> | ||
| <artifactId>commons-cli</artifactId> |
There was a problem hiding this comment.
I think this one can be at test scope.
There was a problem hiding this comment.
We expose MiniZooKeeperCluster as IA.Public and it is under src/main, not src/test, so we must declare the dependencies which are only used by zookeeper server at compile/runtime scope...
IIRC there is a issue for removing the class but there are objections from our users...
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Strange, the same UT failed two times in jdk8 pre commit build. Should not be related but let me download the test logs to check. And the PR for branch-2.x will be different as we have two hadoop profiles. Will open another PR for branch-2.x. |
|
Filed HBASE-28180 for the failed UT, it is not related. |
hbase-common/pom.xml
Outdated
| we depend on hadoop-common and hadoop-common depends on curator and | ||
| zookeeper, so we need to include netty4 dependency here | ||
| --> | ||
| <dependency> |
There was a problem hiding this comment.
Sorry, is there a reason this can't come in transitively?
I wonder if we should enable maven dependency analyzer plugin, which would disallow dependencies that aren't used directly (and require explicit dependencies for classes we do use)
There was a problem hiding this comment.
There is a nasty issue for netty that, some dependencies will use netty-all while some others may use something like netty-handler, maven does not know that netty-all includes netty-handler, so it will treat them as different dependencies and then cause runtime class conflicts. So here we will exclude all netty dependencies from our libraries, and then manually include netty-all. You can see what we have done in the root pom file.
There was a problem hiding this comment.
Oh, the newest netty-all module does not include any real classes, just depend on all other netty modules...
But anyway, it could still lead to different versions of netty modules if we rely on transitive dependencies of netty.
There was a problem hiding this comment.
I think we should include netty-bom with scope import and the version we want. This will add all netty modules to our dependency management with the same version. So then any transitive dependencies that depend on any netty module will all use the same version.
There was a problem hiding this comment.
Sounds good. We could try this for netty 4.
There was a problem hiding this comment.
And maybe netty3 is also OK if we have a netty3 dependency in the dependencyManagement section. Let me try to remove the exclusion.
Apache9
force-pushed
the
HBASE-28153
branch
2 times, most recently
from
5175220 to
fab354a
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Zookeeper removed a lot of dependencies by their own, so we do not need to exclude them now. The exclusion section of zookeeper is done when we uses 3.4.x, this is the compile dependencies https://mvnrepository.com/artifact/org.apache.zookeeper/zookeeper/3.4.14 And now https://mvnrepository.com/artifact/org.apache.zookeeper/zookeeper/3.8.3 I think all other exclusions are netty related. So do you have any other concerns? |
bbeaudreault
approved these changes
Oct 31, 2023
Apache9
added a commit
that referenced
this pull request
Nov 1, 2023
Apache9
added a commit
that referenced
this pull request
Nov 1, 2023
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit a973739)
Apache9
added a commit
to Apache9/hbase
that referenced
this pull request
Nov 1, 2023
Backport of apache#5475 (cherry picked from commit 0d04a60)
Apache9
added a commit
to Apache9/hbase
that referenced
this pull request
Nov 1, 2023
Backport of apache#5475 (cherry picked from commit 0d04a60)
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Nov 2, 2023
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
prabhjyotsingh
added a commit
to acceldata-io/hbase
that referenced
this pull request
Apr 3, 2024
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit a973739)
prabhjyotsingh
added a commit
to acceldata-io/hbase
that referenced
this pull request
Apr 3, 2024
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit a973739)
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…e#5490) Backport of apache#5475 (cherry picked from commit 0d04a60) Signed-off-by: Andrew Purtell <apurtell@apache.org> Change-Id: Idb37274228e07afeea5518a1c79d4a053be46a05
shubhluck
added a commit
to acceldata-io/hbase
that referenced
this pull request
May 2, 2024
* HBASE-27694 * Hbase Jackson-mapper-asl dependency removed, jettison/avro/netty upgrade, hadoop tag-release 3.2.3.3.2.2.0-1095 * ODP-1203 - Avro upgrade to 1.11.3 * ODP-1210|netty version upgrade to 4.1.94 * zookeeper release version change to 3.2.2.0-1095 * distribution management addition (cherry picked from commit 39171e4) * hadoop.guava.version upgrade 32.0-jre (cherry picked from commit e1e87b1) * removed additional jackson-mapper-asl dependency from hbase-shaded-testing-util * HBASE-28022 Remove netty 3 dependency in the pom file for hbase-endpoint (apache#5351) Signed-off-by: Xin Sun <ddupgs@gmail.com> Signed-off-by: GeorryHuang <huangzhuoyue@apache.org> (cherry picked from commit 5464245) * HBASE-28153 Upgrade zookeeper to a newer version (apache#5475) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit a973739) * Hbase Jackson-mapper-asl dependency removed, jettison/avro/netty upgrade, hadoop tag-release 3.2.3.3.2.2.0-1095 * Hbase Jackson-mapper-asl dependency removed, jettison/avro/netty upgrade, hadoop tag-release 3.2.3.3.2.2.0-1095 * Distribution management addition * Fixed version as per main across all poms --------- Co-authored-by: manishsinghmowall <manishsingh@acceldata.io> Co-authored-by: kravii <ravi@acceldata.io> Co-authored-by: Duo Zhang <zhangduo@apache.org> Co-authored-by: Prabhjyot Singh <prabhjyot@acceldata.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.