-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[FLINK-38912] Upgrade Netty to 4.2.6.Final #27469
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
base: master
Are you sure you want to change the base?
Conversation
27c738a to
f046713
Compare
ferenc-csaky
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.
Thanks for taking care of this. Logic LGTM, just left some questions before approve.
flink-python/pom.xml
Outdated
| <scope>runtime</scope> | ||
| </dependency> | ||
|
|
||
| <!-- Netty dependency is fixed to 4.1 here as Arrow expects this. It should be removed or updated once Apache Arrow version is upgraded. --> |
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.
Can we also include the current Arrow version here so it will be more obvious in the future which Arrow version was the cause for this without browsing mvnrepo and its dependency matrix? :)
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.
Yes, I've added a comment. Unfortunately there are no currently released versions of Apache Arrow that's built against Netty 4.2.x, the upgrade was done 3 weeks ago: apache/arrow-java@7d4cf21
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 mean to include the arrow version explicitly to the comment that will resolve the current situation. Something like:
"... It should be removed or updated once Apache Arrow is upgraded to version x.y.z+."
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.
Sorry, I've left this comment and updated it locally, just did not push until I've finished looking into your other comment. I've pushed the update now.
flink-rpc/flink-rpc-akka/pom.xml
Outdated
| <!-- Only .class files from flink-shaded-netty should be loaded, so don't package any to flink-rpc-akka jar. --> | ||
| <exclude>**/*.class</exclude> |
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.
Was this wrong before, or is it something new to handle?
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.
The issue here was that some Netty classes required by Pekko were not properly shaded as netty-codec was reworked in the latest Netty release. Not sure why, but depending on netty-all will result in a lot of classes, such as LengthFieldPrepender not being included in the shading step.
My idea here was to not package any shaded Netty dependencies from this module, and instead only use the already shaded classes from flink-shaded-netty. This would be inconsistent compared to how this module worked before, so I've gone with another fix:
- Depend only on Netty dependencies that are required by Pekko (this reduces the amount of dependencies and fixes the shading issue mentioned)
- Update licenses, as they were quite outdated.
I think this makes the most sense, as we don't need ALL of Netty dependencies coming from netty-all, as we can be quite certain of what Pekko actually needs by looking at their dependencies.
...ueryable-state-runtime/src/test/java/org/apache/flink/queryablestate/network/ClientTest.java
Show resolved
Hide resolved
f046713 to
3d3d871
Compare
What is the purpose of the change
Upgrade Netty to 4.2.6.Final to match the version of flink-shaded. This is meant to compliment PR #27407.
Brief change log
Verifying this change
CI should pass
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation