HDDS-14673. Implement ScmEnumCodec without reflection.#9843
HDDS-14673. Implement ScmEnumCodec without reflection.#9843szetszwo merged 9 commits intoapache:masterfrom
Conversation
|
Hi @szetszwo, could you please help review this PR when you have time? Thanks! |
There was a problem hiding this comment.
@Russole , thanks a lot for spending time on finding out all the required enums!
We may change ScmEnumCodec to support generic enums; see https://issues.apache.org/jira/secure/attachment/13080998/9843_review.patch
szetszwo
changed the title
|
Thanks @szetszwo for the review. I’ve updated the changes based on the comments. |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
|
Hi @szetszwo.I’ve updated the patch to fix the enum deserialization logic in ScmEnumCodec. Previously we decoded using the enum ordinal (values[n]), but we serialize protobuf enums with getNumber(), so this could map to the wrong value. The new implementation restores the correct semantics by looking up enum constants by protobuf number (via a prebuilt number -> enum map), and keeps the same wire format. Could you please take another look when you have time? Thanks! |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@Russole , thanks for catching the bug of using values array! You are right that we should use protobuf enum number.
Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13081015/9843_review2.patch
| public final class ScmCodecFactory { | ||
|
|
||
| private static Map<Class<?>, ScmCodec> codecs = new HashMap<>(); | ||
| private static final Map<Class<?>, ScmCodec<?>> CODECS = new HashMap<>(); |
There was a problem hiding this comment.
Sorry that I added final in my last review. Let's do it later in order to avoid renaming.
| 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<>(); |
There was a problem hiding this comment.
Let's pass the forNumber method:
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;
}| + StringUtils.bytes2String(value.asReadOnlyByteBuffer()), e); | ||
| } | ||
|
|
||
| final T decoded = byNumber.get(n); |
There was a problem hiding this comment.
Use forNumber:
final T decoded = forNumber.apply(n);
|
Thanks @szetszwo for the comments. I’ve updated the changes accordingly. |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
2 participants
What changes were proposed in this pull request?
ProtocolMessageEnumhandling fromScmCodecFactory.HddsProtos.PipelineStateHddsProtos.LifeCycleEventHddsProtos.NodeTypeScmCodecFactory.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14673
How was this patch tested?