szetszwo commented on code in PR #9952:
URL: https://github.com/apache/ozone/pull/9952#discussion_r2967097085
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java:
##########
@@ -104,8 +104,9 @@ public Message encode() throws
InvalidProtocolBufferException {
// Set actual method parameter type, not actual argument type.
// This is done to avoid MethodNotFoundException in case if argument is
// subclass type, where as method is defined with super class type.
- argBuilder.setType(parameterTypes[paramCounter++].getName());
- argBuilder.setValue(ScmCodecFactory.getCodec(argument.getClass())
+ final String parameterType = parameterTypes[paramCounter++].getName();
+ argBuilder.setType(parameterType);
+ argBuilder.setValue(ScmCodecFactory.getInstance().getCodec(parameterType)
Review Comment:
When the class is available, we should getCodec using the class.
BTW, could you also remove this args list? We should just use the proto
builder.
```java
public Message encode() throws InvalidProtocolBufferException {
final SCMRatisRequestProto.Builder requestProtoBuilder =
SCMRatisRequestProto.newBuilder();
requestProtoBuilder.setType(type);
final Method.Builder methodBuilder = Method.newBuilder();
methodBuilder.setName(operation);
for (int i = 0; i < parameterTypes.length; i++) {
// Set actual method parameter type, not actual argument type.
// This is done to avoid MethodNotFoundException in case if argument is
// subclass type, where as method is defined with super class type.
final Class<?> parameterType = parameterTypes[i];
methodBuilder.addArgs(MethodArgument.newBuilder()
.setType(parameterType.getName())
.setValue(FACTORY.getCodec(parameterType).serialize(arguments[i]))
.build());
}
requestProtoBuilder.setMethod(methodBuilder.build());
final SCMRatisRequestProto requestProto = requestProtoBuilder.build();
return Message.valueOf(requestProto.toByteString());
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java:
##########
@@ -74,35 +76,35 @@ public final class ScmCodecFactory {
putEnum(NodeType.class, NodeType::forNumber);
// Must be the last one
- final ClassResolver resolver = new ClassResolver(codecs.keySet());
+ resolver = new ClassResolver(codecs.keySet());
codecs.put(List.class, new ScmListCodec(resolver));
}
- static <T extends Message> void putProto(T proto) {
+ private <T extends Message> void putProto(T proto) {
codecs.put(proto.getClass(),
new ScmNonShadedGeneratedMessageCodec<>(proto.getParserForType()));
}
- static <T extends Enum<T> & ProtocolMessageEnum> void putEnum(
+ private <T extends Enum<T> & ProtocolMessageEnum> void putEnum(
Class<T> enumClass, IntFunction<T> forNumber) {
codecs.put(enumClass, new ScmEnumCodec<>(enumClass, forNumber));
}
- private ScmCodecFactory() { }
+ public static ScmCodecFactory getInstance() {
+ return INSTANCE;
+ }
- public static ScmCodec getCodec(Class<?> type)
+ public ScmCodec getCodec(String className)
Review Comment:
Let's keep the parameter declared using Class.
For the className case, we should resolve the class first.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java:
##########
@@ -150,9 +151,10 @@ public static SCMRatisRequest decode(Message message)
throw new InvalidProtocolBufferException("Missing argument value");
}
try {
+ final String argumentType = argument.getType();
final Class<?> clazz = ReflectionUtil.getClass(argument.getType());
parameterTypes[paramCounter++] = clazz;
- args.add(ScmCodecFactory.getCodec(clazz)
+ args.add(ScmCodecFactory.getInstance().getCodec(argumentType)
Review Comment:
Use factory to resolve the class:
```java
final String argumentType = argument.getType();
final Class<?> clazz = FACTORY.resolve(argumentType);
parameterTypes[paramCounter++] = clazz;
args.add(FACTORY.getCodec(clazz).deserialize(argument.getValue()));
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java:
##########
@@ -124,27 +126,28 @@ Class<?> get(String className) throws
InvalidProtocolBufferException {
if (c != null) {
return c;
}
- throw new InvalidProtocolBufferException("Class not found for " +
className);
- }
- Class<?> get(Class<?> clazz) throws InvalidProtocolBufferException {
- final String className = clazz.getName();
Review Comment:
Let's have a private get(Class<?> clazz, String className) method:
```java
Class<?> get(String className) throws InvalidProtocolBufferException {
return get(null, className);
}
Class<?> get(Class<?> clazz) throws InvalidProtocolBufferException {
return get(clazz, clazz.getName());
}
private Class<?> get(Class<?> clazz, String className) throws
InvalidProtocolBufferException {
Objects.requireNonNull(className, "className == null");
final Class<?> c = provided.get(className);
if (c != null) {
return c;
}
final Class<?> found = resolved.get(className);
if (found != null) {
return found;
}
if (clazz == null) {
try {
clazz = Class.forName(className);
} catch (ClassNotFoundException e) {
throw new InvalidProtocolBufferException("Class not found for " +
className, e);
}
}
for (Class<?> base : provided.values()) {
if (base.isAssignableFrom(clazz)) {
resolved.put(className, base);
return base;
}
}
throw new InvalidProtocolBufferException("Failed to resolve " +
className);
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisResponse.java:
##########
@@ -103,8 +104,9 @@ public static SCMRatisResponse decode(RaftClientReply reply)
}
try {
- final Class<?> type = ReflectionUtil.getClass(responseProto.getType());
- return new SCMRatisResponse(ScmCodecFactory.getCodec(type)
+ final String typeName = responseProto.getType();
+ final Class<?> type = ReflectionUtil.getClass(typeName);
Review Comment:
We should use the factory to resolve it.
```java
final Class<?> type = FACTORY.resolve(responseProto.getType());
return new
SCMRatisResponse(FACTORY.getCodec(type).deserialize(responseProto.getValue()));
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisResponse.java:
##########
@@ -73,9 +73,10 @@ public static Message encode(final Object result)
}
final Class<?> type = result.getClass();
+ final String typeName = type.getName();
Review Comment:
Pass the declaration type as a parameter:
```java
public static Message encode(Object result, Class<?> type) throws
InvalidProtocolBufferException {
if (result == null) {
return Message.EMPTY;
}
final SCMRatisResponseProto response = SCMRatisResponseProto.newBuilder()
.setType(type.getName())
.setValue(FACTORY.getCodec(type).serialize(result))
.build();
return
Message.valueOf(UnsafeByteOperations.unsafeWrap(response.toByteString().asReadOnlyByteBuffer()));
}
```
--
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]
