GH-39265: [Java] Make it run well with the netty newest version 4.1.104 by panbingkun · Pull Request #39266 · apache/arrow

panbingkun · 2023-12-18T02:54:05Z

Describe the enhancement requested

When I used netty arrow memory 14.0.1 and netty 4.1.104.Final in Spark, the following error occurred,
After pr: netty/netty#13613, PoolArena no longer extends SizeClasses, but instead uses it as one of its fields, as follows:

in order to ensure that netty arrow memory 14.0.1 works well with netty 4.1.104.Final version, I suggest making similar modifications here.
1.Compilation errors are as follows:
https://github.com/panbingkun/spark/actions/runs/7237466030/job/19717162391

2.Some bugs have been fixed in netty 4.1.104.Final as follows:

4.1.104.Final release note: https://netty.io/news/2023/12/15/4-1-104-Final.html
4.1.103.Final release note: https://netty.io/news/2023/12/13/4-1-103-Final.html
4.1.101.Final release note: https://netty.io/news/2023/11/09/4-1-101-Final.html

Component(s)

Java

Closes: [Java] Make it run well with the netty newest version 4.1.104 #39265

github-actions · 2023-12-18T02:54:30Z

⚠️ GitHub issue #39265 has been automatically assigned in GitHub to PR creator.

lidavidm · 2023-12-18T13:06:38Z

Hmm, dropping compatibility with older versions of Netty isn't great either, if people are using older gRPC. But we might not have a choice. CC @jduo

lidavidm · 2023-12-19T14:09:14Z

CC @danepitkin this means we will have a hard compatibility break between netty versions

danepitkin · 2023-12-19T14:53:06Z

I wonder if it is possible to support both old and new netty versions, maybe using run time reflection?

panbingkun · 2023-12-20T00:36:09Z

I think using the reflection mechanism can solve compatibility issues with older versions, but there may be some performance losses. Can we accept this?

jbonofre · 2023-12-20T12:51:30Z

I'm doing a pass on this one as I'm interested by.

jbonofre

The change looks good to me. I would like to test in Flight context to be sure it's clear with gRPC.

lidavidm · 2023-12-20T14:00:42Z

I think reflection is OK for InnerAllocator() because it's a constructor and shouldn't be in a hot path. It may also be OK in newDirectBufferL, we'd want a benchmark.

That said, reflection might just make modularization more annoying.

lidavidm · 2023-12-20T14:01:24Z

Well, we already use reflection here so that's moot.

lidavidm · 2023-12-20T14:04:57Z

That said, it seems there's a different way to get chunk size? If we can validate that they're the same, why not use that instead and avoid poking internals? https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L663-L671

jbonofre · 2023-12-20T14:58:57Z

@lidavidm yeah, that's my point about buffer allocator. I know gRPC is using that internally as well. It makes sense to use the "regular" netty API instead of tweaking by the internals. I can work on this if you want 😄 (I have some PRs on the way 😄 ).

lidavidm · 2023-12-20T15:06:01Z

@jbonofre if you have the interest then by all means. It is much appreciated.

jbonofre · 2023-12-20T15:07:37Z

@lidavidm my pleasure ! Thanks !

panbingkun · 2023-12-21T02:43:32Z

From a coding perspective, PooledByteBufAllocator#metric()#chunkSize() and PooledByteBufAllocator#directArenas(0)#chunkSize() are indeed the same, as follows:

1.PooledByteBufAllocator#metric()#chunkSize()
1.1.PooledByteBufAllocator#metric()
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L569-L571

https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L330

1.2.PooledByteBufAllocatorMetric#chunkSize()
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocatorMetric.java#L30-L32

https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocatorMetric.java#L96-L98

1.3.PooledByteBufAllocator#chunkSize()
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L669-L671

https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L283

2.PooledByteBufAllocator#directArenas(0)#sizeClass#chunkSize()
2.1.PooledByteBufAllocator#directArenas
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L316-L322

https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PoolArena.java#L670-L674

https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/SizeClasses.java#L179

https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/SizeClasses.java#L99

…/arrow into upgrade_netty_4_1_104

panbingkun · 2023-12-21T07:22:54Z

java/memory/memory-netty/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java


    private final PoolArena<ByteBuffer>[] directArenas;
    private final MemoryStatusThread statusThread;
-    private final int chunkSize;


From code analysis, it seems that the chunkSize field here is useless. After the constructor is assigned, it is no longer used, and this value is directly derived from the parent class. So I suggest removing it.

panbingkun · 2023-12-21T07:25:43Z

java/memory/memory-netty/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java

      if (directArena != null) {

-        if (initialCapacity > directArena.chunkSize) {
+        if (initialCapacity > chunkSize()) {


The value of directArena.chunkSize is actually equal to chunkSize(), and it is a final.

Thanks. One final thing, I think chunkSize is deprecated in Netty? Is it possible to use the non-deprecated version?

panbingkun · 2023-12-21T07:27:32Z

After completing the above changes, I found that it can run well with both netty version 4.1.104 and netty version 4.1.100.

Based on my poor understanding of netty arrow memory , if there is anything wrong, please correct me, Thanks all!

danepitkin · 2023-12-21T19:53:17Z

Thanks for finding a way to support old versions of netty!

conbench-apache-arrow · 2023-12-23T00:54:28Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit a4a3d3f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

Make it run well with the netty newest version 4.1.104

6423ff4

panbingkun requested a review from lidavidm as a code owner December 18, 2023 02:54

github-actions bot added Component: Java awaiting review Awaiting review labels Dec 18, 2023

panbingkun mentioned this pull request Dec 18, 2023

[SPARK-46432][BUILD] Upgrade Netty to 4.1.106.Final apache/spark#44384

Closed

kou changed the title ~~GH-39265: [JAVA ]Make it run well with the netty newest version 4.1.104~~ GH-39265: [Java] Make it run well with the netty newest version 4.1.104 Dec 18, 2023

Merge branch 'main' into upgrade_netty_4_1_104

fcc442a

jbonofre approved these changes Dec 20, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 20, 2023

panbingkun added 3 commits December 21, 2023 10:56

Make it run well with the netty newest version 4.1.104

0bfe61c

Merge branch 'upgrade_netty_4_1_104' of https://github.com/panbingkun…

1248aca

…/arrow into upgrade_netty_4_1_104

Make it run well with the netty newest version 4.1.104

587f918

panbingkun commented Dec 21, 2023

View reviewed changes

github-actions bot removed the awaiting committer review Awaiting committer review label Dec 21, 2023

github-actions bot added the awaiting changes Awaiting changes label Dec 21, 2023

lidavidm merged commit a4a3d3f into apache:main Dec 22, 2023

lidavidm removed the awaiting changes Awaiting changes label Dec 22, 2023

AbdulR3hman mentioned this pull request Jan 5, 2024

build(deps): bump apache.arrow.version from 13.0.0 to 14.0.2 awslabs/aws-athena-query-federation#1668

Closed

Conversation

panbingkun commented Dec 18, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the enhancement requested

Component(s)

Uh oh!

github-actions bot commented Dec 18, 2023

Uh oh!

lidavidm commented Dec 18, 2023

Uh oh!

lidavidm commented Dec 19, 2023

Uh oh!

danepitkin commented Dec 19, 2023

Uh oh!

panbingkun commented Dec 20, 2023

Uh oh!

jbonofre commented Dec 20, 2023

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Dec 20, 2023

Uh oh!

lidavidm commented Dec 20, 2023

Uh oh!

lidavidm commented Dec 20, 2023

Uh oh!

jbonofre commented Dec 20, 2023

Uh oh!

lidavidm commented Dec 20, 2023

Uh oh!

jbonofre commented Dec 20, 2023

Uh oh!

panbingkun commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panbingkun Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

lidavidm Dec 21, 2023

Choose a reason for hiding this comment

Uh oh!

panbingkun commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danepitkin commented Dec 21, 2023

Uh oh!

conbench-apache-arrow bot commented Dec 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

panbingkun commented Dec 18, 2023 •

edited by github-actions bot

Loading

panbingkun commented Dec 21, 2023 •

edited

Loading

panbingkun Dec 21, 2023 •

edited

Loading

panbingkun commented Dec 21, 2023 •

edited

Loading