-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-14259. Improve TestManagedDirectSlice. #9571
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
base: master
Are you sure you want to change the base?
Conversation
|
@szetszwo thanks for the patch! |
sreejasahithi
left a comment
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.
Thanks @szetszwo for working on this. LGTM
swamirishi
left a comment
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.
@szetszwo thanks for working on the patch I have left a comment inline about the test coverage.
| final byte[] bytes = new byte[size]; | ||
| RANDOM.nextBytes(bytes); | ||
| try (CodecBuffer buffer = CodecBuffer.allocateDirect(size).put(ByteBuffer.wrap(bytes)); | ||
| ManagedDirectSlice directSlice = new ManagedDirectSlice(buffer.asReadOnlyByteBuffer()); |
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.
We should also test a byte buffer with a non zero position to test the slice initiailization in the constructor. Consider the case if someone removes the slice logic then things would break without a test case failure. If there is some logic change we should ensure we have test case for that.
BTW I have raised a patch on rocksdb which gives us a way to avoid the slicing and with this constructor we can directly pass the position and remaining to the JNI layer in a single call.
facebook/rocksdb#14208
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.
Thanks for making the changes to the patch @szetszwo LGTM
What changes were proposed in this pull request?
What is the link to the Apache JIRA
HDDS-14259
How was this patch tested?
This is a test only change.