szetszwo commented on code in PR #9843:
URL: https://github.com/apache/ozone/pull/9843#discussion_r2867513948
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmEnumCodec.java:
##########
@@ -19,35 +19,53 @@
import com.google.common.primitives.Ints;
import com.google.protobuf.ProtocolMessageEnum;
-import java.lang.reflect.InvocationTargetException;
-import org.apache.hadoop.hdds.scm.ha.ReflectionUtil;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.hadoop.hdds.StringUtils;
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.UnsafeByteOperations;
+import org.apache.ratis.util.Preconditions;
/**
- * {@link ScmCodec} for {@link ProtocolMessageEnum} objects.
+ * {@link ScmCodec} for protobuf {@link ProtocolMessageEnum} objects.
+ * Stores protobuf enum number (wire value) and restores via number lookup.
*/
-public class ScmEnumCodec implements ScmCodec<Object> {
+class ScmEnumCodec<T extends Enum<T> & ProtocolMessageEnum> implements
ScmCodec<T> {
+ private final Class<T> enumClass;
+ private final Map<Integer, T> byNumber = new HashMap<>();
Review Comment:
Let's pass the forNumber method:
```java
private final Class<T> enumClass;
private final IntFunction<T> forNumber;
ScmEnumCodec(Class<T> enumClass, IntFunction<T> forNumber) {
Preconditions.assertTrue(enumClass.isEnum(), () -> "Not an enum: " +
enumClass);
for (T constant : enumClass.getEnumConstants()) {
Preconditions.assertSame(constant,
forNumber.apply(constant.getNumber()), "constant");
}
this.enumClass = enumClass;
this.forNumber = forNumber;
}
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmEnumCodec.java:
##########
@@ -19,35 +19,53 @@
import com.google.common.primitives.Ints;
import com.google.protobuf.ProtocolMessageEnum;
-import java.lang.reflect.InvocationTargetException;
-import org.apache.hadoop.hdds.scm.ha.ReflectionUtil;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.hadoop.hdds.StringUtils;
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.UnsafeByteOperations;
+import org.apache.ratis.util.Preconditions;
/**
- * {@link ScmCodec} for {@link ProtocolMessageEnum} objects.
+ * {@link ScmCodec} for protobuf {@link ProtocolMessageEnum} objects.
+ * Stores protobuf enum number (wire value) and restores via number lookup.
*/
-public class ScmEnumCodec implements ScmCodec<Object> {
+class ScmEnumCodec<T extends Enum<T> & ProtocolMessageEnum> implements
ScmCodec<T> {
+ private final Class<T> enumClass;
+ private final Map<Integer, T> byNumber = new HashMap<>();
+
+ ScmEnumCodec(Class<T> enumClass) {
+ Preconditions.assertTrue(enumClass.isEnum(), "enumClass is not an enum: "
+ enumClass);
+ this.enumClass = enumClass;
+
+ // Build number -> enum constant map (no reflection)
+ for (T e : enumClass.getEnumConstants()) {
+ byNumber.put(e.getNumber(), e);
+ }
+ }
@Override
- public ByteString serialize(Object object)
- throws InvalidProtocolBufferException {
- // toByteArray returns a new array
- return
UnsafeByteOperations.unsafeWrap(Ints.toByteArray(((ProtocolMessageEnum)
object).getNumber()));
+ public ByteString serialize(T object) {
+ return
UnsafeByteOperations.unsafeWrap(Ints.toByteArray(object.getNumber()));
}
@Override
- public Object deserialize(Class<?> type, ByteString value)
- throws InvalidProtocolBufferException {
+ public T deserialize(Class<?> type, ByteString value) throws
InvalidProtocolBufferException {
+ final int n;
try {
- return ReflectionUtil.getMethod(type, "valueOf", int.class)
- .invoke(null, Ints.fromByteArray(
- value.toByteArray()));
- } catch (NoSuchMethodException | IllegalAccessException
- | InvocationTargetException ex) {
+ n = Ints.fromByteArray(value.toByteArray());
+ } catch (Exception e) {
+ throw new InvalidProtocolBufferException(
+ "Failed to deserialize enum " + enumClass + ": "
+ + StringUtils.bytes2String(value.asReadOnlyByteBuffer()), e);
+ }
+
+ final T decoded = byNumber.get(n);
Review Comment:
Use forNumber:
```java
final T decoded = forNumber.apply(n);
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java:
##########
@@ -35,22 +36,29 @@
*/
public final class ScmCodecFactory {
- private static Map<Class<?>, ScmCodec> codecs = new HashMap<>();
+ private static final Map<Class<?>, ScmCodec<?>> CODECS = new HashMap<>();
Review Comment:
Sorry that I added final in my last review. Let's do it later in order to
avoid renaming.
--
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]
