HDDS-14238. Move RDBBatchOperation Byte comparison to native comparison for optimization #9550

swamirishi · 2025-12-24T13:24:40Z

What changes were proposed in this pull request?

Instead of performing ByteWise comparison on Java side moving the comparison to native side would more optimal since most of the transactions are going to use direct byte buffers. Here we intend to use the rocksdb Slice to perform bytewise comparisons. RocksDB Bytewise comparators also uses the same comparator.
https://github.com/facebook/rocksdb/blob/c110091d368b8a01b5be36a14198769e60786c05/util/comparator.cc#L36-L38
Unfortunately currently in 7.7.3 there is no direct native comparison that has been implemented. We can look into contributing to Rocksdb by having a JNI implementation for

https://github.com/facebook/rocksdb/blob/0bf9079d44eea91afda7151306d3a3439a39511b/java/src/main/java/org/rocksdb/NativeComparatorWrapper.java#L16

Till then this is a nice workaround to have.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14238

How was this patch tested?

Update existing unit tests

…on for optimization Change-Id: Ia7655ff5148197be488a2c1151ec7fd1d6f9d452

swamirishi · 2025-12-28T16:56:58Z

@szetszwo This is good to be reviewed. I will click on ready for review when I get a +1. I don't want to run the CI multiple times.

szetszwo

@swamirishi , thanks for working on this! Please see the comments inlined.

szetszwo · 2025-12-28T22:02:32Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

+    private void initWithByteArray(byte[] array) {
+      this.slice = new ManagedSlice(array);
      this.hash = ByteBuffer.wrap(array).hashCode();
    }

-    ByteBuffer asReadOnlyByteBuffer() {
-      return buffer.asReadOnlyByteBuffer();
+    private void initWithDirectByteBuffer(ByteBuffer byteBuffer) {
+      this.slice = new ManagedDirectSlice(byteBuffer);
+      this.hash = byteBuffer.hashCode();
    }


Constructors should not call any functions. Otherwise, the fields cannot be final, which is very useful for reducing bugs and for understanding the code..

private final AbstractSlice<?> slice; /** Cache the hash value. */ private final int hash; static Bytes newBytes(CodecBuffer buffer) { Objects.requireNonNull(buffer, "buffer == null"); return buffer.isDirect() ? new Bytes(buffer.asReadOnlyByteBuffer()) : new Bytes(buffer.getArray()); } Bytes(ByteBuffer buffer) { Objects.requireNonNull(buffer, "buffer == null"); assertTrue(buffer.isDirect(), "buffer is not direct"); this.slice = new ManagedDirectSlice(buffer); this.hash = buffer.hashCode(); } Bytes(byte[] array) { Objects.requireNonNull(array, "array == null"); this.slice = new ManagedSlice(array); this.hash = ByteBuffer.wrap(array).hashCode(); }

szetszwo · 2025-12-28T22:10:07Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

    public String toString() {
-      return array != null ? bytes2String(array)
-          : bytes2String(asReadOnlyByteBuffer());
+      return slice.toString();


Have you checked what will it return? Could you show an example output?

public static void main(String[] args) throws CodecException { String value = "test"; Bytes bytes = new Bytes(value.getBytes(UTF_8)); System.out.println("To String Heap Value: " + bytes); Bytes dbytes = newBytes(CodecBufferCodec.get(true).fromPersistedFormat(value.getBytes(UTF_8))); System.out.println("To String Direct Value: " + dbytes); }

2025-12-28 18:25:34,786 [main] INFO db.CodecBuffer (CodecBuffer.java:set(85)) - Successfully set constructor to LeakDetector::newCodecBuffer: org.apache.hadoop.hdds.utils.db.CodecBuffer$$Lambda$4/1694556038@7085bdee To String Heap Value: test To String Direct Value: test

To String Heap Value: test
To String Direct Value: test

Isn't the data supposed to be binary? This seems not working.

It is supposed to be the char decoded value. https://github.com/facebook/rocksdb/blob/1cfdece85ddc9450d24c3007222195e46e1cc0a8/util/slice.cc#L303-L304

When you say it is not working. Maybe you are not loading the rocksdb libs before this. I forgot there was static codeblock loading the rocksdb lib.
This should work:

public static void main(String[] args) throws CodecException { ManagedRocksObjectUtils.loadRocksDBLibrary(); String value = "test"; Bytes bytes = new Bytes(value.getBytes(UTF_8)); System.out.println("To String Heap Value: " + bytes); Bytes dbytes = newBytes(CodecBufferCodec.get(true).fromPersistedFormat(value.getBytes(UTF_8))); System.out.println("To String Direct Value: " + dbytes); }

The output before the change and after will be the same since StringUtils.byte2String() also decodes the byte value to String value

If we want the binary value then we have to print the hex value.
slice.toString(true) would do that

I did the above to keep the behaviour same as before.

You are right -- it is the same as before.

szetszwo · 2025-12-28T22:32:34Z

hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/TestCodec.java

-      assertEquals(fromArray.hashCode(), fromBuffer.hashCode());
-      assertEquals(fromArray, fromBuffer);
-      assertEquals(fromBuffer, fromArray);
+    for (CodecBuffer.Allocator allocator : ImmutableList.of(CodecBuffer.Allocator.HEAP, CodecBuffer.Allocator.DIRECT)) {


Pass allocator instead of adding a loop.

runTestBytes(original, codec, CodecBuffer.Allocator.HEAP); runTestBytes(original, codec, CodecBuffer.Allocator.DIRECT); } static <T> void runTestBytes(T object, Codec<T> codec, CodecBuffer.Allocator allocator) throws IOException { final byte[] array = codec.toPersistedFormat(object); final Bytes fromArray = new Bytes(array); try (CodecBuffer buffer = codec.toCodecBuffer(object, allocator)) {

szetszwo · 2025-12-28T22:55:42Z

... I will click on ready for review when I get a +1. I don't want to run the CI multiple times.

@swamirishi , we should pass the CI first before asking for reviewing -- if the change cannot pass CI, it may be fundamentally incorrect. What is the point for reviewing?

⚠️ Also, reviewer's time/effort is much more important than saving the machines' time/effort.

Change-Id: I913be571b87e318abc798c7396ca072de23b01e8

swamirishi · 2025-12-28T23:29:50Z

... I will click on ready for review when I get a +1. I don't want to run the CI multiple times.

@swamirishi , we should pass the CI first before asking for reviewing -- if the change cannot pass CI, it may be fundamentally incorrect. What is the point for reviewing?

⚠️ Also, reviewer's time/effort is much more important than saving the machines' time/effort.

One can always check the CI run on the fork. We have been following this model on almost all PRs for a while.
@adoroszlai can also add his opinions on this.

szetszwo · 2025-12-29T00:13:08Z

One can always check the CI run on the fork. ...

Please provide the link then. I would like to save my time for finding it.

swamirishi · 2025-12-29T07:40:32Z

One can always check the CI run on the fork. ...

Please provide the link then. I would like to save my time for finding it.

https://github.com/swamirishi/ozone/actions/runs/20561058088

swamirishi · 2025-12-29T11:41:05Z

@szetszwo i have addressed all your review comments is this good to be merged?

szetszwo

@swamirishi , thanks for the update! Please see the comment inlined.

szetszwo · 2025-12-29T17:15:21Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBBatchOperation.java

    public String toString() {
-      return array != null ? bytes2String(array)
-          : bytes2String(asReadOnlyByteBuffer());
+      return slice.toString();


To String Heap Value: test
To String Direct Value: test

Isn't the data supposed to be binary? This seems not working.

szetszwo

+1 the change looks good.

swamirishi · 2025-12-29T18:19:33Z

Thank you @szetszwo for reviewing the patch

swamirishi requested review from jojochuang and szetszwo December 24, 2025 13:24

swamirishi force-pushed the HDDS-14238 branch 2 times, most recently from e4e5016 to 692b5ed Compare December 24, 2025 13:51

swamirishi mentioned this pull request Dec 24, 2025

HDDS-14235. Cleanup RDBBatchOperation. #9551

Closed

swamirishi force-pushed the HDDS-14238 branch 8 times, most recently from a3349d7 to deb7900 Compare December 25, 2025 05:47

HDDS-14238. Move RDBBatchOperation Byte comparison to native comparis…

72c13eb

…on for optimization Change-Id: Ia7655ff5148197be488a2c1151ec7fd1d6f9d452

swamirishi force-pushed the HDDS-14238 branch from deb7900 to 72c13eb Compare December 28, 2025 16:53

szetszwo reviewed Dec 28, 2025

View reviewed changes

HDDS-14238. Address review comments

159d35b

Change-Id: I913be571b87e318abc798c7396ca072de23b01e8

swamirishi requested a review from szetszwo December 28, 2025 23:31

swamirishi marked this pull request as ready for review December 29, 2025 09:32

szetszwo reviewed Dec 29, 2025

View reviewed changes

swamirishi requested a review from szetszwo December 29, 2025 17:52

szetszwo approved these changes Dec 29, 2025

View reviewed changes

swamirishi merged commit 64bb019 into apache:master Dec 29, 2025
95 of 96 checks passed

HDDS-14238. Move RDBBatchOperation Byte comparison to native comparison for optimization #9550

HDDS-14238. Move RDBBatchOperation Byte comparison to native comparison for optimization #9550

Uh oh!

Conversation

swamirishi commented Dec 24, 2025

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

swamirishi commented Dec 28, 2025

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swamirishi Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swamirishi Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo commented Dec 28, 2025

Uh oh!

swamirishi commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo commented Dec 29, 2025

Uh oh!

swamirishi commented Dec 29, 2025

Uh oh!

swamirishi commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

szetszwo Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi commented Dec 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

szetszwo Dec 29, 2025 •

edited

Loading

swamirishi Dec 29, 2025 •

edited

Loading

swamirishi Dec 29, 2025 •

edited

Loading

swamirishi commented Dec 28, 2025 •

edited

Loading

swamirishi commented Dec 29, 2025 •

edited

Loading

szetszwo Dec 29, 2025 •

edited

Loading