Skip to content

Conversation

@szetszwo
Copy link
Contributor

What changes were proposed in this pull request?

  • The offset, which is not useful anymore after HDDS-14237, should be removed.
  • The hardcoded test cases are too simple: too few and all the sizes are power of 2. We should test some random sizes.

What is the link to the Apache JIRA

HDDS-14259

How was this patch tested?

This is a test only change.

@szetszwo szetszwo requested a review from swamirishi December 29, 2025 20:58
@rich7420
Copy link
Contributor

@szetszwo thanks for the patch!

Copy link
Contributor

@sreejasahithi sreejasahithi left a 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

Copy link
Contributor

@swamirishi swamirishi left a 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());
Copy link
Contributor

@swamirishi swamirishi Dec 30, 2025

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

Copy link
Contributor

@swamirishi swamirishi left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants