-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-28879 Bump hbase-thirdparty to 4.1.9 #6295
Conversation
I talked myself into it. Here's a backport for branch-2.5. |
Oops nope, that a major version bump of the protobuf jar. |
Thanks @ndimiduk for taking care of this. There is an issue for this HBASE-28879. |
Perfect. Thanks. |
This comment has been minimized.
This comment has been minimized.
@@ -931,7 +931,7 @@ | |||
databind] must be kept in sync with the version of jackson-jaxrs-json-provider shipped in | |||
hbase-thirdparty. | |||
--> | |||
<hbase-thirdparty.version>4.1.8</hbase-thirdparty.version> | |||
<hbase-thirdparty.version>4.1.9</hbase-thirdparty.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may also want to sync versions of error prone and netty4 with hbase-thirdparty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting out of hand. See how many fix-up commits I had to push to various projects? We should be managing all these version numbers with a BOM import.
This blog post has a nice write-up on the idea, https://www.garretwilson.com/blog/2023/06/14/improve-maven-bom-pattern
I think that hbase-thirdparty could publish a BOM pom file that can be imported into any of the downstream hbase projects that make use of that release of hbase-thirdparty. That will centralize management of these dependencies in the hbase-thirdparty repo, and we won't have to play whack-a-mole in the main project poms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was thinking about the same. Because it does not make sense to waste time doing this and still miss out on one or the another. We already have something similar for org.mockito:mockito-bom:
Line 1626 in 449c446
<artifactId>mockito-bom</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should file a jira to handle this. At least will have a better way of doing thirdparty change with a one liner change with next thirdparty release, which is how it should have been ideally.
I can help with implementing this, please lmk if you do not plan to fix this yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating error prone is not trivial, sometimes it may cause some compile errors since they may introduced some new check rules, so usually I will open a new issue for updating error prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And on netty4 dependencies, hbase does not depend on it directly since we use the shaded one in hbase thirdparty, it is mainly introduced by hadoop and zookeeper. And in hadoop 3.4.0, IIRC hadoop also shaded and relocated netty in their thirdparty jar, so maybe we need to discuss whether we will need to force netty dependencies in hbase, maybe just leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And on netty4 dependencies, hbase does not depend on it directly since we use the shaded one in hbase thirdparty, it is mainly introduced by hadoop and zookeeper. And in hadoop 3.4.0, IIRC hadoop also shaded and relocated netty in their thirdparty jar, so maybe we need to discuss whether we will need to force netty dependencies in hbase, maybe just leave it as is.
So we need not necessarily align netty with thirdparty.
Same for error prone.
Please see apache/hbase-thirdparty#124, NihalJain@0b51c71
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so we do not need to maintain alignment on netty or errorprone? Jackson and protobuf are the only two that require alignment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me explain more clear.
For error prone, we'd better also bump the error prone to the same version with thirparty, but it is not trivial sometimes, and since hbase-thirdparty only depends on the annotation jar(even not shaded), so it is not likely to introduce any conflicts. So I think it is better to have a separated issue for bumping it, after upgrading the hbase-thirdparty.
For netty, since hbase does not depend on netty4 directly, we do not need to align the netty version with the one in hbase-thirdparty.
We maintain it in our pom is because the conflicts between zookeeper and hadoop. So if there are no CVEs for netty, we do not need to bump it in hbase. And after hadoop 3.4.0, since hadoop also shade netty(IIRC), maybe we even do not need to do this any more. If there are new CVEs for netty, maybe we just need to bump the zookeeper dependency?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9d9bb38
to
c72cc80
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c72cc80
to
a37b900
Compare
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
FYI @Apache9