-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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
[SPARK-44128][BUILD] Upgrade netty to 4.1.93 #41681
Conversation
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.
+1, LGTM(pending test)
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.
Seems there are api changes in the new netty version. Can you fix it or maybe we need wait until arrow-netty upgrade? @panbingkun
This version has been released on the official website, but it is not yet available in Maven Repo. Let's wait for a moment. 😄 |
What I mean is that we may need to wait for the next arrow version to be compatible with the netty 4.1.94.Final |
Let's upgrade to the |
@@ -212,7 +212,7 @@ | |||
<commons-cli.version>1.5.0</commons-cli.version> | |||
<bouncycastle.version>1.60</bouncycastle.version> | |||
<tink.version>1.9.0</tink.version> | |||
<netty.version>4.1.92.Final</netty.version> | |||
<netty.version>4.1.93.Final</netty.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.
I think we should add some comments to inform other developers not to try upgrading to 4.1.94 and need to wait for arrow-memory-netty to upgrade together
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.
Done
https://github.com/apache/arrow/pull/36211/files arrow already upgrade Netty to 4.1.94.Final and this may be released in arrow 13.0, I'm not sure if this is available because it also updates the grpc to 1.56.0, which is different from Spark Connect using a different grpc version On the other hand, Netty 4.1.94 fixes a CVE apache/arrow#36211, I want to know if this CVE will affect Spark? |
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.
+1, LGTM (pending CI)
friendly ping @srowen @dongjoon-hyun further review I think we can upgrade to use netty 4.1.93 first, the versions greater than 4.1.94 need to wait for |
I agree with you, @LuciferYang and @panbingkun , because 4.1.93.Final is the first release for Java 21 official support. |
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.
+1, LGTM.
Merged to master because this is already tested via 432f665 |
Thank you for working on this, @LuciferYang and @panbingkun ! |
FYI the next |
What changes were proposed in this pull request?
This pr aims to upgrade netty from 4.1.92 to 4.1.93.
Why are the changes needed?
1.v4.1.92 VS v4.1.93
netty/netty@netty-4.1.92.Final...netty-4.1.93.Final
2.The new version brings some bug fix, eg:
3.The release notes as follows:
4.Why not upgrade to
4-1-94-Final
version?Because the return value of the 'threadCache()' (from
PoolThreadCache
toPoolArenasCache
) method of the netty Inner class used in the 'arrow memory netty' version '12.0.1' has changed and belongs to break change, let's wait for the upgrade of the 'arrow memory netty' before upgrading to the '4-1-94-Final' version.The reference is as follows:
https://github.com/apache/arrow/blob/6af660f48472b8b45a5e01b7136b9b040b185eb1/java/memory/memory-netty/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java#L164
https://github.com/netty/netty/blob/da1a448d5bc4f36cc1744db93fcaf64e198db2bd/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L732-L736
Does this PR introduce any user-facing change?
No
How was this patch tested?
Pass GA.