HDDS-14561. SCMRatisRequest/ResponseProto should use the shaded protobuf from Ratis by Russole · Pull Request #9733 · apache/ozone

Russole · 2026-02-08T15:20:01Z

What changes were proposed in this pull request?

Updated SCM Ratis request/response serialization and deserialization
to use Ratis shaded protobuf (ByteString / ByteBuffer) at the transport boundary.
Removed unnecessary intermediate byte[] conversions in the SCM Ratis message path.
Updated existing unit tests to match the revised serialization and deserialization
implementation.

What is the link to the Apache JIRA

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

How was this patch tested?

All CI checks passed.
- https://github.com/Russole/ozone/actions/runs/21776638347

…buf from Ratis

szetszwo

@Russole , thanks for working on this!

Let see if the current change can be compiled -- I think we need to change the protobuf compiler or use shading to make SCMRatisRequest/Response extending the protobuf inside Ratis.

Russole · 2026-02-09T00:23:15Z

Thanks @szetszwo!

The current change compiles and all CI checks pass:
https://github.com/Russole/ozone/actions/runs/21776638347

Agreed that SCMRatisRequestProto and SCMRatisResponseProto still extend
com.google.protobuf.*, as they are generated by the standard protobuf compiler.
This PR focuses on the SCM↔Ratis message boundary, updating the wrapping/parsing
to use Ratis shaded protobuf types and avoid extra byte[] conversions.

Let me know if you think it makes sense to open a follow-up JIRA for generating
the SCM Ratis protos with the shaded protobuf.

szetszwo · 2026-02-09T03:47:40Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java

+        UnsafeByteOperations.unsafeWrap(
+            requestProto.toByteString().asReadOnlyByteBuffer()));


@Russole , I see how this change works now -- it does prevent copying to an array but still needs type conversion from com.google.protobuf.ByteString to org.apache.ratis.thirdparty.com.google.protobuf.ByteString.

Let's fix also the conversion in this JIRA; see ContainerCommandRequestProto as an example for how to do it.

unsafeWrap is not needed anymore.

return Message.valueOf(requestProto.toByteString());

Russole · 2026-02-10T04:11:41Z

Thanks for the clarification!

I’m working on this now. I see the issue with the remaining type conversion between
com.google.protobuf and the Ratis shaded protobuf, and I’m updating the implementation
to follow the same pattern as ContainerCommandRequestProto so that the SCM Ratis
request/response fully use the shaded protobuf types.

I’ll update the PR once this is addressed.

szetszwo

@Russole , thanks for the great work! I believe that you have spent a lot time on this.

Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13080780/9733_review.patch

szetszwo · 2026-02-11T18:26:01Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ByteStringCodec.java

+    if (object instanceof ByteString) {
+      return (ByteString) object;
+    }
+    if (object instanceof com.google.protobuf.ByteString) {


Do we still need to keep supporting com.google.protobuf.ByteString? Should everything used in SCM Codec have changed to org.apache.ratis.thirdparty.com.google.protobuf.ByteString after this PR?

szetszwo · 2026-02-11T18:33:41Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java

+    codecs.put(com.google.protobuf.ByteString.class, new ByteStringCodec());
+    codecs.put(org.apache.ratis.thirdparty.com.google.protobuf.ByteString.class,


If com.google.protobuf.ByteString is still needed,

Add a new class, say ScmByteStringCodec, for org.apache.ratis.thirdparty.com.google.protobuf.ByteString.

import org.apache.ratis.thirdparty.com.google.protobuf.ByteString.

codecs.put(com.google.protobuf.ByteString.class, new ByteStringCodec()); codecs.put(ByteString.class, new ScmByteStringCodec());

If com.google.protobuf.ByteString is not needed, just remove it.

szetszwo · 2026-02-11T18:41:55Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/EnumCodec.java

    // toByteArray returns a new array
-    return ProtoUtils.unsafeByteString(Ints.toByteArray(
-        ((ProtocolMessageEnum) object).getNumber()));
+    return UnsafeByteOperations.unsafeWrap(Ints.toByteArray(((ProtocolMessageEnum) object).getNumber()));


FYI, filed HDDS-14623 for removing ProtoUtils.

szetszwo · 2026-02-11T18:50:08Z

...op-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/GeneratedMessageCodec.java


  @Override
-  public ByteString serialize(Object object) {
-    return ((Message)object).toByteString();


Do we need to support both com.google.protobuf.Message and org.apache.ratis.thirdparty.com.google.protobuf.Message?

If yes, use two GeneratedMessageCodec classes instead of using one class. Also, we need to put them to CodecFactory.

//CodecFactory codecs.put(com.google.protobuf.Message.class, new GeneratedMessageCodec()); codecs.put(Message.class, new ScmGeneratedMessageCodec());

szetszwo · 2026-02-11T18:59:42Z

...op-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ManagedSecretKeyCodec.java

+      throw new org.apache.ratis.thirdparty.com.google.protobuf.InvalidProtocolBufferException(
+          e.getMessage());


Let's include the type in the message and use e as the cause (it will keep e's stack trace.)

throw new InvalidProtocolBufferException("Failed to deserialize value for " + type, e);

szetszwo · 2026-02-11T19:01:26Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java

+        UnsafeByteOperations.unsafeWrap(
+            requestProto.toByteString().asReadOnlyByteBuffer()));


unsafeWrap is not needed anymore.

return Message.valueOf(requestProto.toByteString());

szetszwo · 2026-02-11T19:10:03Z

...erver-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisProtocolCompatibility.java

+  private static byte[] bytesOf(com.google.protobuf.ByteString b) {
+    return b.toByteArray();
+  }
+
+  private static byte[] bytesOf(org.apache.ratis.thirdparty.com.google.protobuf.ByteString b) {
+    return b.toByteArray();
+  }


Use the following assertByteStringEquals and remove the bytesOf methods.

private static void assertByteStringEquals(com.google.protobuf.ByteString proto2, ByteString proto3) { assertEquals(UnsafeByteOperations.unsafeWrap(proto2.asReadOnlyByteBuffer()), proto3); }

szetszwo · 2026-02-11T19:14:09Z

...erver-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisProtocolCompatibility.java

+  private static String shortDebugStringProto2(
+      com.google.protobuf.MessageOrBuilder msg) {
+    return com.google.protobuf.TextFormat.shortDebugString(msg);
+  }
+
+  private static String shortDebugStringProto3(
+      org.apache.ratis.thirdparty.com.google.protobuf.MessageOrBuilder msg) {
+    return org.apache.ratis.thirdparty.com.google.protobuf.TextFormat.shortDebugString(msg);
+  }


Use assertShortDebugString below:

private static void assertShortDebugString(com.google.protobuf.MessageOrBuilder proto2, org.apache.ratis.thirdparty.com.google.protobuf.MessageOrBuilder proto3) { assertEquals(com.google.protobuf.TextFormat.shortDebugString(proto2), org.apache.ratis.thirdparty.com.google.protobuf.TextFormat.shortDebugString(proto3)); }

szetszwo · 2026-02-11T19:39:31Z

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisResponse.java

    when(reply.getMessage()).thenReturn(Message.valueOf(
-        org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom(
-            proto.toByteArray())));
+        org.apache.ratis.thirdparty.com.google.protobuf.UnsafeByteOperations


import UnsafeByteOperations.

szetszwo · 2026-02-11T19:41:09Z

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisRequest.java

    Message msg = Message.valueOf(
-        org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom(
-            proto.toByteArray()));
+        org.apache.ratis.thirdparty.com.google.protobuf.UnsafeByteOperations


import UnsafeByteOperations.

Russole · 2026-02-12T15:03:06Z

Thanks @szetszwo for the detailed review and the patch. I really appreciate your time and guidance.
I’ll review the comments and apply the suggested changes, and will update the PR shortly.

szetszwo

@Russole , thanks for the update! Just have some minor comments inlined.

szetszwo · 2026-02-13T17:44:10Z

...op-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/GeneratedMessageCodec.java

+    final byte[] bytes = ((Message) object).toByteString().toByteArray();
+    return ByteString.copyFrom(bytes);


Use unsafeWarp as below?

UnsafeByteOperations.unsafeWrap(((Message) object).toByteString().asReadOnlyByteBuffer());

szetszwo · 2026-02-13T17:45:01Z

...op-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/GeneratedMessageCodec.java

-  public Message deserialize(Class<?> type, ByteString value)
-      throws InvalidProtocolBufferException {
+  public Object deserialize(Class<?> type, ByteString value)
+      throws org.apache.ratis.thirdparty.com.google.protobuf.InvalidProtocolBufferException {


Keep throws InvalidProtocolBufferException since the import is changed.

szetszwo · 2026-02-13T17:49:11Z

...erver-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisProtocolCompatibility.java

  static final Class<?>[] TYPES = {String.class, Integer.class, byte[].class};

-  static <T> ByteString randomValue(Class<T> clazz) {
+  static <T> org.apache.ratis.thirdparty.com.google.protobuf.ByteString randomValueProto3(Class<T> clazz) {


ByteString is already imported.

szetszwo · 2026-02-13T17:49:42Z

...erver-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisProtocolCompatibility.java

+      return org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFromUtf8(string);
    } else if (clazz == Integer.class) {
      final ByteBuffer buffer = ByteBuffer.allocate(4);
      buffer.putInt(RANDOM.nextInt());
-      return ByteString.copyFrom(buffer.array());
+      return org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom(buffer.array());
    } else if (clazz == byte[].class) {
      final byte[] bytes = new byte[RANDOM.nextInt(3)];
      RANDOM.nextBytes(bytes);
-      return ByteString.copyFrom(bytes);
+      return org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom(bytes);


ByteString is already imported.

szetszwo · 2026-02-13T17:51:18Z

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisRequest.java

@@ -145,8 +145,8 @@ public void testDecodeMissingMethodNameShouldFail() throws Exception {
            .build();

    Message msg = Message.valueOf(
-        org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom(
-            proto.toByteArray()));
+        org.apache.ratis.thirdparty.com.google.protobuf.UnsafeByteOperations
+            .unsafeWrap(proto.toByteString().asReadOnlyByteBuffer()));

    InvalidProtocolBufferException ex = assertThrows(
        InvalidProtocolBufferException.class,
@@ -173,8 +173,8 @@ public void testDecodeMissingArgumentTypeShouldFail() throws Exception {
            .build();

    Message msg = Message.valueOf(
-        org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom(
-            proto.toByteArray()));
+        org.apache.ratis.thirdparty.com.google.protobuf.UnsafeByteOperations


import UnsafeByteOperations.

…n up imports

szetszwo

@Russole , thanks for the update! Just one last comment inlined.

szetszwo · 2026-02-14T02:56:08Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ByteStringCodec.java

  public Object deserialize(Class<?> type, ByteString value)
      throws InvalidProtocolBufferException {
-    return value;
+    return com.google.protobuf.ByteString.copyFrom(value.toByteArray());


Use com.google.protobuf.UnsafeByteOperations.

Russole added 2 commits February 7, 2026 15:23

HDDS-14561. SCMRatisRequest/ResponseProto should use the shaded proto…

e87984a

…buf from Ratis

Fix checkstyle error

c0b3145

szetszwo reviewed Feb 8, 2026

View reviewed changes

szetszwo reviewed Feb 9, 2026

View reviewed changes

Russole added 4 commits February 11, 2026 00:28

Apply review feedback on protobuf handling

4aa8930

Merge branch 'master' into HDDS-14561

eaa58a0

Fix shaded protobuf mismatch in TestSCMRatisProtocolCompatibility test

8eeaa6e

Fix unit test error

298f5de

Russole requested a review from szetszwo February 11, 2026 17:25

szetszwo reviewed Feb 11, 2026

View reviewed changes

Russole added 2 commits February 14, 2026 00:40

Refactor implementation and update CodecFactory

ac43a62

Fix checkstyle error

2351cd1

szetszwo reviewed Feb 13, 2026

View reviewed changes

Add ByteString handling support, adopt UnsafeByteOperations, and clea…

df9720b

…n up imports

Russole requested a review from szetszwo February 14, 2026 00:15

szetszwo reviewed Feb 14, 2026

View reviewed changes

		UnsafeByteOperations.unsafeWrap(
		requestProto.toByteString().asReadOnlyByteBuffer()));

		codecs.put(com.google.protobuf.ByteString.class, new ByteStringCodec());
		codecs.put(org.apache.ratis.thirdparty.com.google.protobuf.ByteString.class,

		throw new org.apache.ratis.thirdparty.com.google.protobuf.InvalidProtocolBufferException(
		e.getMessage());

		final byte[] bytes = ((Message) object).toByteString().toByteArray();
		return ByteString.copyFrom(bytes);

Conversation

Russole commented Feb 8, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Russole commented Feb 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Russole commented Feb 10, 2026

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!

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!

Russole commented Feb 12, 2026

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!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants