szetszwo commented on code in PR #9733:
URL: https://github.com/apache/ozone/pull/9733#discussion_r2794838396
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/CodecFactory.java:
##########
@@ -47,7 +46,9 @@ public final class CodecFactory {
codecs.put(Boolean.class, new BooleanCodec());
codecs.put(BigInteger.class, new BigIntegerCodec());
codecs.put(X509Certificate.class, new X509CertificateCodec());
- codecs.put(ByteString.class, new ByteStringCodec());
+ codecs.put(com.google.protobuf.ByteString.class, new ByteStringCodec());
+
codecs.put(org.apache.ratis.thirdparty.com.google.protobuf.ByteString.class,
Review Comment:
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.
```java
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.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ByteStringCodec.java:
##########
@@ -28,12 +28,28 @@ public class ByteStringCodec implements Codec {
@Override
public ByteString serialize(Object object)
throws InvalidProtocolBufferException {
+ if (object instanceof ByteString) {
+ return (ByteString) object;
+ }
+ if (object instanceof com.google.protobuf.ByteString) {
Review Comment:
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?
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisProtocolCompatibility.java:
##########
@@ -268,10 +298,20 @@ static void runTestProto3ResponseCanBeParsedByProto2()
throws Exception {
Proto2SCMRatisProtocolForTesting.SCMRatisResponseProto.parseFrom(proto3.toByteArray());
assertEquals(proto3.getType(), proto2.getType());
- assertEquals(proto3.getValue(), proto2.getValue());
+ assertArrayEquals(bytesOf(proto3.getValue()), bytesOf(proto2.getValue()));
assertEquals(proto2.toString(), proto3.toString());
- assertEquals(TextFormat.shortDebugString(proto2),
TextFormat.shortDebugString(proto3));
+ assertEquals(shortDebugStringProto2(proto2),
shortDebugStringProto3(proto3));
assertEquals(proto3,
SCMRatisProtocol.SCMRatisResponseProto.parseFrom(proto2.toByteArray()));
}
+
+ 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);
+ }
Review Comment:
Use assertShortDebugString below:
```java
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));
}
```
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisProtocolCompatibility.java:
##########
@@ -155,20 +176,28 @@ static void runTestProto2RequestCanBeParsedByProto3(
}
assertEquals(proto2.toString(), proto3.toString());
- assertEquals(TextFormat.shortDebugString(proto2),
TextFormat.shortDebugString(proto3));
+ assertEquals(shortDebugStringProto2(proto2),
shortDebugStringProto3(proto3));
assertEquals(proto2,
Proto2SCMRatisProtocolForTesting.SCMRatisRequestProto.parseFrom(proto3.toByteArray()));
}
+ 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();
+ }
Review Comment:
Use the following assertByteStringEquals and remove the bytesOf methods.
```java
private static void assertByteStringEquals(com.google.protobuf.ByteString
proto2, ByteString proto3) {
assertEquals(UnsafeByteOperations.unsafeWrap(proto2.asReadOnlyByteBuffer()),
proto3);
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java:
##########
@@ -111,9 +112,10 @@ public Message encode() throws
InvalidProtocolBufferException {
}
methodBuilder.addAllArgs(args);
requestProtoBuilder.setMethod(methodBuilder.build());
+ final SCMRatisRequestProto requestProto = requestProtoBuilder.build();
return Message.valueOf(
- org.apache.ratis.thirdparty.com.google.protobuf.ByteString.copyFrom(
- requestProtoBuilder.build().toByteArray()));
+ UnsafeByteOperations.unsafeWrap(
+ requestProto.toByteString().asReadOnlyByteBuffer()));
Review Comment:
unsafeWrap is not needed anymore.
```java
return Message.valueOf(requestProto.toByteString());
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/EnumCodec.java:
##########
@@ -34,8 +34,7 @@ public class EnumCodec implements Codec {
public ByteString serialize(Object object)
throws InvalidProtocolBufferException {
// toByteArray returns a new array
- return ProtoUtils.unsafeByteString(Ints.toByteArray(
- ((ProtocolMessageEnum) object).getNumber()));
+ return
UnsafeByteOperations.unsafeWrap(Ints.toByteArray(((ProtocolMessageEnum)
object).getNumber()));
Review Comment:
FYI, filed HDDS-14623 for removing ProtoUtils.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisRequest.java:
##########
@@ -105,8 +105,8 @@ public void testDecodeMissingRequestTypeShouldFail() 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
Review Comment:
import UnsafeByteOperations.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMRatisResponse.java:
##########
@@ -108,8 +108,8 @@ public void testResponseDecodeMissingTypeShouldFail()
throws Exception {
RaftClientReply reply = mock(RaftClientReply.class);
when(reply.isSuccess()).thenReturn(true);
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
Review Comment:
import UnsafeByteOperations.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/GeneratedMessageCodec.java:
##########
@@ -17,34 +17,52 @@
package org.apache.hadoop.hdds.scm.ha.io;
-import com.google.protobuf.ByteString;
-import com.google.protobuf.InvalidProtocolBufferException;
-import com.google.protobuf.Message;
-import java.lang.reflect.InvocationTargetException;
+import java.nio.ByteBuffer;
import org.apache.hadoop.hdds.scm.ha.ReflectionUtil;
+import org.apache.ratis.thirdparty.com.google.protobuf.ByteString;
+import
org.apache.ratis.thirdparty.com.google.protobuf.InvalidProtocolBufferException;
+import org.apache.ratis.thirdparty.com.google.protobuf.Message;
/**
* {@link Codec} for {@link Message} objects.
*/
public class GeneratedMessageCodec implements Codec {
@Override
- public ByteString serialize(Object object) {
- return ((Message)object).toByteString();
Review Comment:
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.
```java
//CodecFactory
codecs.put(com.google.protobuf.Message.class, new
GeneratedMessageCodec());
codecs.put(Message.class, new ScmGeneratedMessageCodec());
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ManagedSecretKeyCodec.java:
##########
@@ -30,14 +31,21 @@ public class ManagedSecretKeyCodec implements Codec {
public ByteString serialize(Object object)
throws InvalidProtocolBufferException {
ManagedSecretKey secretKey = (ManagedSecretKey) object;
- return secretKey.toProtobuf().toByteString();
+ return UnsafeByteOperations.unsafeWrap(
+ secretKey.toProtobuf().toByteString().asReadOnlyByteBuffer());
}
@Override
public Object deserialize(Class<?> type, ByteString value)
throws InvalidProtocolBufferException {
- SCMSecretKeyProtocolProtos.ManagedSecretKey message =
- SCMSecretKeyProtocolProtos.ManagedSecretKey.parseFrom(value);
- return ManagedSecretKey.fromProtobuf(message);
+ try {
+ SCMSecretKeyProtocolProtos.ManagedSecretKey message =
+ SCMSecretKeyProtocolProtos.ManagedSecretKey.parseFrom(
+ value.asReadOnlyByteBuffer());
+ return ManagedSecretKey.fromProtobuf(message);
+ } catch (com.google.protobuf.InvalidProtocolBufferException e) {
+ throw new
org.apache.ratis.thirdparty.com.google.protobuf.InvalidProtocolBufferException(
+ e.getMessage());
Review Comment:
Let's include the type in the message and use e as the cause (it will keep
e's stack trace.)
```java
throw new InvalidProtocolBufferException("Failed to deserialize value
for " + type, e);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
