-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Upgrade Netty to 4.1.x #6417
Upgrade Netty to 4.1.x #6417
Conversation
This is being proposed because I'm doing some gRPC extensions internally. And the gRPC java libraries require netty 4.1 |
#5059 no longer an issue? |
@leventov I don't have a great way to tell right now (we don't use http emitter internally) but looking at #4973 (comment) I was able to pinpoint the offending library causing all kinds of netty versions to be brought in. It was upgraded to a version that uses netty 4.1.29-Final |
The dependencies now look like this: https://gist.github.com/drcrallen/848d56a9aa409e15aab350b63eed7bd5 |
Noteably the following still have "old" netty dependencies:
|
I'm going to explicitly exclude the rocket mq dependency on netty-all in the extension |
@himanshug do you happen to have a way to reproduce the prior error in the new build environment? java util is part of the main druid now. |
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.
I put a Release Notes tag since we don't have a "might need more validation during release" kind of tag |
@drcrallen please don't merge such important PRs with just one approve and without waiting for 3 working days (so that everybody has a chance to review it), as the current policy prescribes. I would say this PR should have been labelled |
Whop, sorry about that |
On a side not, I don't see how a version change qualifies as a design review. Can you explain the thought process there more? |
I mean, this is an important decision that more than one party (apart from the PR author) should agree with. At Metamarkets we decided that we could upgrade to Netty 4.1, but already after this PR was merged. And we still don't know what people who care about Hadoop support (@nishantmonu51 / @b-slim) think |
FWIW we will definitely be putting this through the paces hadoop-support wise before voting on the 0.13.0 release. So if it causes anything bad to happen then we will speak up then. @nishantmonu51 / @b-slim probably will too. |
+1 from my side, If we face compatibility issues in our internal testing with the RC, will raise this again. |
Spark 2.3.0 upgrades Netty support to a 4.1 branch as per apache/spark#19884 . This PR upgrades various dependencies to all use Netty 4.1.x.
Solves #4390
((assuming the extension upgrades to spark 2.3 or greater))