You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2023/12/18 02:54:08 UTC
[PR] GH-39265: [JAVA ]Make it run well with the netty newest version 4.1.104 [arrow]
panbingkun opened a new pull request, #39266:
URL: https://github.com/apache/arrow/pull/39266
### 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: https://github.com/netty/netty/pull/13613, `PoolArena` no longer extends `SizeClasses`, but instead uses it as one of its fields, as follows:
<img width="1051" alt="image" src="https://github.com/apache/arrow/assets/15246973/6112757b-b2b7-42aa-b4c1-6ab473b91a09">
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
<img width="1005" alt="image" src="https://github.com/apache/arrow/assets/15246973/98edb6a1-f0e6-4d4e-b568-fbdbffe612f0">
2.Some bugs have been fixed in `netty 4.1.104.Final` as follows:
<img width="862" alt="image" src="https://github.com/apache/arrow/assets/15246973/12354a1e-cddd-4ab8-b168-e92712d84cea">
<img width="861" alt="image" src="https://github.com/apache/arrow/assets/15246973/bd7d27e1-3953-451c-8c9b-24ecb0d61efd">
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #39266:
URL: https://github.com/apache/arrow/pull/39266#discussion_r1434087537
##########
java/memory/memory-netty/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java:
##########
@@ -166,7 +163,7 @@ private UnsafeDirectLittleEndian newDirectBufferL(int initialCapacity, int maxCa
if (directArena != null) {
- if (initialCapacity > directArena.chunkSize) {
+ if (initialCapacity > chunkSize()) {
Review Comment:
Thanks. One final thing, I think chunkSize is deprecated in Netty? Is it possible to use the non-deprecated version?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1863661541
I think using the reflection mechanism can solve compatibility issues with older versions, but there may be some `performance losses`. Can we accept this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1864631876
@jbonofre if you have the interest then by all means. It is much appreciated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1868156667
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit a4a3d3f4825eb025657121e70c9d86e8d6ecff35.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/19908359570) has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [JAVA ]Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1859469173
:warning: GitHub issue #39265 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #39266:
URL: https://github.com/apache/arrow/pull/39266#discussion_r1433651593
##########
java/memory/memory-netty/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java:
##########
@@ -166,7 +163,7 @@ private UnsafeDirectLittleEndian newDirectBufferL(int initialCapacity, int maxCa
if (directArena != null) {
- if (initialCapacity > directArena.chunkSize) {
+ if (initialCapacity > chunkSize()) {
Review Comment:
The value of `directArena.chunkSize` is actually equal to `chunkSize()`, and it is a final.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1864422809
I'm doing a pass on this one as I'm interested by.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #39266:
URL: https://github.com/apache/arrow/pull/39266
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1862826749
CC @danepitkin this means we will have a hard compatibility break between netty versions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1865749471
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1864524976
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1864525900
Well, we already use reflection here so that's moot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #39266:
URL: https://github.com/apache/arrow/pull/39266#discussion_r1433648470
##########
java/memory/memory-netty/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java:
##########
@@ -137,7 +137,6 @@ private class InnerAllocator extends PooledByteBufAllocator {
private final PoolArena<ByteBuffer>[] directArenas;
private final MemoryStatusThread statusThread;
- private final int chunkSize;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1862915302
I wonder if it is possible to support both old and new netty versions, maybe using run time reflection?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1864634550
@lidavidm my pleasure ! Thanks !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #39266:
URL: https://github.com/apache/arrow/pull/39266#discussion_r1433648470
##########
java/memory/memory-netty/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java:
##########
@@ -137,7 +137,6 @@ private class InnerAllocator extends PooledByteBufAllocator {
private final PoolArena<ByteBuffer>[] directArenas;
private final MemoryStatusThread statusThread;
- private final int chunkSize;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1865398052
From a coding perspective, `PooledByteBufAllocator#metric()#chunkSize()` and `PooledByteBufAllocator#directArenas(0)#chunkSize()` are indeed the same, as follows:
1.PooledByteBufAllocator#metric()#chunkSize()
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L569-L571
<img width="470" alt="image" src="https://github.com/apache/arrow/assets/15246973/98695a33-8d27-461e-bdab-55a002780871">
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L330
<img width="516" alt="image" src="https://github.com/apache/arrow/assets/15246973/02a1924b-f6f9-4a51-bf1e-e07cf8ddd62e">
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocatorMetric.java#L30-L32
<img width="590" alt="image" src="https://github.com/apache/arrow/assets/15246973/5bb51a2d-8fc7-44a2-8533-b14b75695856">
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocatorMetric.java#L96-L98
<img width="389" alt="image" src="https://github.com/apache/arrow/assets/15246973/effde511-9304-41b8-a84f-4a8e9022c01c">
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L283
<img width="615" alt="image" src="https://github.com/apache/arrow/assets/15246973/5322de38-3051-4ccd-a751-7bb0ab51a74f">
https://github.com/netty/netty/blob/427ace4cdd86cce84e467ec0a8a81ca022ed6f72/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L669-L671
<img width="618" alt="image" src="https://github.com/apache/arrow/assets/15246973/8de9b023-9554-412c-b64c-7f239eb48cf2">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1860452226
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1864531255
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1864620164
@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 😄 ).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39265: [Java] Make it run well with the netty newest version 4.1.104 [arrow]
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on PR #39266:
URL: https://github.com/apache/arrow/pull/39266#issuecomment-1866848152
Thanks for finding a way to support old versions of netty!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org