Skip to content

HDDS-8542. In RDBTable, add a put method using RocksDB ByteBuffer APIs.#4666

Merged
szetszwo merged 4 commits intoapache:masterfrom
szetszwo:HDDS-8542
May 7, 2023
Merged

HDDS-8542. In RDBTable, add a put method using RocksDB ByteBuffer APIs.#4666
szetszwo merged 4 commits intoapache:masterfrom
szetszwo:HDDS-8542

Conversation

@szetszwo
Copy link
Contributor

@szetszwo szetszwo commented May 5, 2023

What changes were proposed in this pull request?

  • Add a put method to use RocksDB ByteBuffer APIs.
  • Support ByteBuffer methods in Codec and some of the implementations. (Will work on other implementations in other JIRAs.)

What is the link to the Apache JIRA

HDDS-8542

How was this patch tested?

Add new tests to test the codec implementations.

@szetszwo szetszwo requested review from ChenSammi and adoroszlai May 6, 2023 17:35
Copy link
Contributor

@adoroszlai adoroszlai 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 the patch, looks very solid.

Comment on lines +91 to +103
runTest(codec, original, original.length());
}
gc();
}

static <T> void runTest(Codec<T> codec, T original,
Integer serializedSize) throws Exception {
Assertions.assertTrue(codec.supportCodecBuffer());

// serialize to byte[]
final byte[] array = codec.toPersistedFormat(original);
if (serializedSize != null) {
Assertions.assertEquals(serializedSize, array.length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would fail if String has multibyte chars. Should pass String.getBytes(UTF_8).length instead of String.length(). This is not a problem with the current state of the code, since runTest is only called with Latin1 strings, but could be improved in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point! Thanks.

@szetszwo szetszwo merged commit 75f5a28 into apache:master May 7, 2023
@szetszwo
Copy link
Contributor Author

szetszwo commented May 7, 2023

@adoroszlai , thanks a lot for reviewing this!

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.

2 participants