-
Notifications
You must be signed in to change notification settings - Fork 963
Upgrade to Netty 4.1.31 and use individual dependencies #1784
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
Conversation
| </dependency> | ||
| <dependency> | ||
| <groupId>io.netty</groupId> | ||
| <artifactId>netty-codec-socks</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.
Do we need this one?
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 already being pulled by other dependencies (eg: finagle, etc..). Here, I'm not adding a new dependency, just pinning all the artifacts to be on the same version.
| </dependency> | ||
| <dependency> | ||
| <groupId>io.netty</groupId> | ||
| <artifactId>netty-handler-proxy</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.
Do we need this one?
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.
same as above
|
run integration tests |
ivankelly
left a comment
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.
lgtm, one question about the transports
| - lib/io.netty-netty-handler-proxy-4.1.31.Final.jar | ||
| - lib/io.netty-netty-resolver-4.1.31.Final.jar | ||
| - lib/io.netty-netty-resolver-dns-4.1.31.Final.jar | ||
| - lib/io.netty-netty-tcnative-boringssl-static-2.0.19.Final.jar |
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.
Is this packaged as part of -all? I seem to recall transports were separate projects.
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.
That one (the static ssl lib) is not part of -all
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.
netty-tcnative-boringssl-static was already included as a dependency. I have upgrade from 2.0.7.Final to 2.0.19.Final since it's needed by the new Netty version
|
run integration tests |
|
@eolivelli can you please check again? |
eolivelli
left a comment
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.
#shipit
I am preparing 4.8.1.
Do you want to cherry pick?
|
@eolivelli I think it's good to keep it for 4.9.0. The dependency change (from all to individuals) might surprise users in a patch release. Also, there's no specific bug that was fixed in Netty-4.1.31 that we need to make available. |
|
@merlimat okay. |
Motivation
Upgrade to latest Netty version which brings in perf improvements and some new features (eg: Added option to do busy-wait on epoll netty/netty#8267)
Broke down the dependencies from
netty-allinto individual components, as discussed at Configure Netty allocator in bookie and client #1755 (comment)