GH-39265: [Java] Make it run well with the netty newest version 4.1.104

[panbingkun]

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]

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

[lidavidm]

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]

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

[danepitkin]

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

[panbingkun]

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]

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]

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]

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

[lidavidm]

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]

@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]

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

[jbonofre]

@lidavidm my pleasure ! Thanks !

[panbingkun]

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

[panbingkun]

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]

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

[lidavidm]

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

[panbingkun]

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]

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

[conbench-apache-arrow]

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.