-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Fix java.lang.NoSuchMethodError: java.nio.ByteBuffer.position(I)Ljava/nio/ByteBuffer when enabling topic metadata compression #11594
Conversation
…/nio/ByteBuffer when enabling topic metadata compression Related to apache#11593. This PR is copy the bytes to the DirectBuf to get around the issue apache#11593.
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.
LGTM
|
||
byte[] byteArray = managedLedgerInfo.toByteArray(); | ||
uncompressedByteBuf = Unpooled.directBuffer(byteArray.length); | ||
uncompressedByteBuf.writeBytes(byteArray); |
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.
Why we need a copy here?
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.
Please add some comments to avoid being optimized for wrappers later by other developers
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.
Unpooled.wrappedBuffer
will create ByteBuf on heap and the hasMemoryAddress
will return false
.
Unpooled.directBuffer
will create ByteBuf on PooledUnsafeDirectByteBuf
, and the hasMemoryAddress
will return false
.
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.
nice catch!
Please add notes to explain the reason to use Unpooled.directBuffer
instead of Unpooled.wrappedBuffer
|
||
byte[] byteArray = managedLedgerInfo.toByteArray(); | ||
uncompressedByteBuf = Unpooled.directBuffer(byteArray.length); | ||
uncompressedByteBuf.writeBytes(byteArray); |
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.
Unpooled.wrappedBuffer
will create ByteBuf on heap and the hasMemoryAddress
will return false
.
Unpooled.directBuffer
will create ByteBuf on PooledUnsafeDirectByteBuf
, and the hasMemoryAddress
will return false
.
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 ask for another release at airlift?
It looks like they had a bad compatibility issue on the release.
We do are not in a hurry as this feature is for 2.9
@eolivelli Yes, we can ask for a new airlift release, but the code is already in branch-2.8, we need to find a way to get around first to unblock 2.8.1 release. |
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.
Makes sense to me
The created issue airlift/aircompressor#133 for tracking fix from aircompressor |
…/nio/ByteBuffer when enabling topic metadata compression (apache#11594) ### Motivation Related to apache#11593. This PR is copying the bytes to the DirectBuf to get around issue apache#11593. The created issue airlift/aircompressor#133 for tracking fix from [aircompressor](https://github.com/airlift/aircompressor) ### Verifying this change Make the `ManagedLedgerCompressionTest` running on JDK8
…/nio/ByteBuffer when enabling topic metadata compression (#11594) ### Motivation Related to #11593. This PR is copying the bytes to the DirectBuf to get around issue #11593. The created issue airlift/aircompressor#133 for tracking fix from [aircompressor](https://github.com/airlift/aircompressor) ### Verifying this change Make the `ManagedLedgerCompressionTest` running on JDK8 (cherry picked from commit 53c7c13)
AirLift has been released |
…r upgrade to 0.20 (#11792) ### Motivation Due to aircompressor 0.19 can't work with heap buffer on JDK1.8, so #11594 use copy data to direct buffer to avoid NoSuchMethodError exception. Now aircompressor released 0.20 and #11790 has upgrade the aircompressor version to 0.20 to fix this issue, we can revert #11594 to avoid copy data to direct buffer to improve performance. ### Modification 1. revert Fix java.lang.NoSuchMethodError: java.nio.ByteBuffer.position(I)Ljava/nio/ByteBuffer when enabling topic metadata compression #11594, but keep the tests.
…r upgrade to 0.20 (#11792) ### Motivation Due to aircompressor 0.19 can't work with heap buffer on JDK1.8, so #11594 use copy data to direct buffer to avoid NoSuchMethodError exception. Now aircompressor released 0.20 and #11790 has upgrade the aircompressor version to 0.20 to fix this issue, we can revert #11594 to avoid copy data to direct buffer to improve performance. ### Modification 1. revert Fix java.lang.NoSuchMethodError: java.nio.ByteBuffer.position(I)Ljava/nio/ByteBuffer when enabling topic metadata compression #11594, but keep the tests. (cherry picked from commit cc1b983)
…/nio/ByteBuffer when enabling topic metadata compression (apache#11594) ### Motivation Related to apache#11593. This PR is copying the bytes to the DirectBuf to get around issue apache#11593. The created issue airlift/aircompressor#133 for tracking fix from [aircompressor](https://github.com/airlift/aircompressor) ### Verifying this change Make the `ManagedLedgerCompressionTest` running on JDK8
…r upgrade to 0.20 (apache#11792) ### Motivation Due to aircompressor 0.19 can't work with heap buffer on JDK1.8, so apache#11594 use copy data to direct buffer to avoid NoSuchMethodError exception. Now aircompressor released 0.20 and apache#11790 has upgrade the aircompressor version to 0.20 to fix this issue, we can revert apache#11594 to avoid copy data to direct buffer to improve performance. ### Modification 1. revert Fix java.lang.NoSuchMethodError: java.nio.ByteBuffer.position(I)Ljava/nio/ByteBuffer when enabling topic metadata compression apache#11594, but keep the tests.
Motivation
Related to #11593. This PR is copying the bytes to the DirectBuf to get around issue #11593.
Verifying this change
Make the
ManagedLedgerCompressionTest
running on JDK8