You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Russole (via GitHub)" <gi...@apache.org> on 2026/03/07 01:16:51 UTC
[PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Russole opened a new pull request, #9878:
URL: https://github.com/apache/ozone/pull/9878
## What changes were proposed in this pull request?
* Identify non-shaded protobuf message classes used in @Replicate method parameters and register them with `ScmNonShadedGeneratedMessageCodec<>(T.parser())`, including element types used in collection parameters such as `List<T>`.
* Based on this investigation, no classes implementing `org.apache.ratis.thirdparty.com.google.protobuf.Message` were found.
* Therefore the generic codec registration codecs.put(Message.class, new `ScmGeneratedMessageCodec())` is removed.
* Change `ScmNonShadedGeneratedMessageCodec` to support generic type
* Add a `ScmNonShadedGeneratedMessageCodec` for each proto class.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14748
## How was this patch tested?
- All CI checks passed
- https://github.com/Russole/ozone/actions/runs/22728755676
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "Russole (via GitHub)" <gi...@apache.org>.
Russole commented on PR #9878:
URL: https://github.com/apache/ozone/pull/9878#issuecomment-4015665777
Thanks @szetszwo for pointing out the exception type. I’ve updated the change accordingly.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9878:
URL: https://github.com/apache/ozone/pull/9878#discussion_r2898982489
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmNonShadedGeneratedMessageCodec.java:
##########
@@ -28,25 +27,25 @@
* {@link ScmCodec} implementation for non-shaded
* {@link com.google.protobuf.Message} objects.
*/
-public class ScmNonShadedGeneratedMessageCodec implements ScmCodec<Object> {
+public class ScmNonShadedGeneratedMessageCodec<T extends Message> implements ScmCodec<T> {
+
+ private final Parser<T> parser;
+
+ public ScmNonShadedGeneratedMessageCodec(Parser<T> parser) {
+ this.parser = parser;
+ }
@Override
- public ByteString serialize(Object object)
- throws InvalidProtocolBufferException {
- return UnsafeByteOperations.unsafeWrap(
- ((Message) object).toByteString().asReadOnlyByteBuffer());
+ public ByteString serialize(T object) throws InvalidProtocolBufferException {
+ return UnsafeByteOperations.unsafeWrap(object.toByteString().asReadOnlyByteBuffer());
}
@Override
- public Object deserialize(Class<?> type, ByteString value)
- throws InvalidProtocolBufferException {
+ public T deserialize(Class<?> type, ByteString value) throws InvalidProtocolBufferException {
try {
- return ReflectionUtil.getMethod(type, "parseFrom", byte[].class)
- .invoke(null, (Object) value.toByteArray());
- } catch (NoSuchMethodException | IllegalAccessException
- | InvocationTargetException ex) {
- ex.printStackTrace();
- throw new InvalidProtocolBufferException("Message cannot be decoded: " + ex.getMessage());
+ return parser.parseFrom(value.toByteArray());
+ } catch (com.google.protobuf.InvalidProtocolBufferException e) {
+ throw new IllegalArgumentException("Message cannot be decoded", e);
Review Comment:
We should throw `org.apache.ratis.thirdparty.com.google.protobuf.InvalidProtocolBufferException`
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9878:
URL: https://github.com/apache/ozone/pull/9878#discussion_r2900685514
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmNonShadedGeneratedMessageCodec.java:
##########
@@ -28,25 +27,25 @@
* {@link ScmCodec} implementation for non-shaded
* {@link com.google.protobuf.Message} objects.
*/
-public class ScmNonShadedGeneratedMessageCodec implements ScmCodec<Object> {
+public class ScmNonShadedGeneratedMessageCodec<T extends Message> implements ScmCodec<T> {
+
+ private final Parser<T> parser;
+
+ public ScmNonShadedGeneratedMessageCodec(Parser<T> parser) {
+ this.parser = parser;
+ }
@Override
- public ByteString serialize(Object object)
- throws InvalidProtocolBufferException {
- return UnsafeByteOperations.unsafeWrap(
- ((Message) object).toByteString().asReadOnlyByteBuffer());
+ public ByteString serialize(T object) throws InvalidProtocolBufferException {
+ return UnsafeByteOperations.unsafeWrap(object.toByteString().asReadOnlyByteBuffer());
}
@Override
- public Object deserialize(Class<?> type, ByteString value)
- throws InvalidProtocolBufferException {
+ public T deserialize(Class<?> type, ByteString value) throws InvalidProtocolBufferException {
try {
- return ReflectionUtil.getMethod(type, "parseFrom", byte[].class)
- .invoke(null, (Object) value.toByteArray());
- } catch (NoSuchMethodException | IllegalAccessException
- | InvocationTargetException ex) {
- ex.printStackTrace();
- throw new InvalidProtocolBufferException("Message cannot be decoded: " + ex.getMessage());
+ return parser.parseFrom(value.toByteArray());
Review Comment:
@Russole , Just find that we should asReadOnlyByteBuffer() here.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #9878:
URL: https://github.com/apache/ozone/pull/9878
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9878:
URL: https://github.com/apache/ozone/pull/9878#discussion_r2900685514
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmNonShadedGeneratedMessageCodec.java:
##########
@@ -28,25 +27,25 @@
* {@link ScmCodec} implementation for non-shaded
* {@link com.google.protobuf.Message} objects.
*/
-public class ScmNonShadedGeneratedMessageCodec implements ScmCodec<Object> {
+public class ScmNonShadedGeneratedMessageCodec<T extends Message> implements ScmCodec<T> {
+
+ private final Parser<T> parser;
+
+ public ScmNonShadedGeneratedMessageCodec(Parser<T> parser) {
+ this.parser = parser;
+ }
@Override
- public ByteString serialize(Object object)
- throws InvalidProtocolBufferException {
- return UnsafeByteOperations.unsafeWrap(
- ((Message) object).toByteString().asReadOnlyByteBuffer());
+ public ByteString serialize(T object) throws InvalidProtocolBufferException {
+ return UnsafeByteOperations.unsafeWrap(object.toByteString().asReadOnlyByteBuffer());
}
@Override
- public Object deserialize(Class<?> type, ByteString value)
- throws InvalidProtocolBufferException {
+ public T deserialize(Class<?> type, ByteString value) throws InvalidProtocolBufferException {
try {
- return ReflectionUtil.getMethod(type, "parseFrom", byte[].class)
- .invoke(null, (Object) value.toByteArray());
- } catch (NoSuchMethodException | IllegalAccessException
- | InvocationTargetException ex) {
- ex.printStackTrace();
- throw new InvalidProtocolBufferException("Message cannot be decoded: " + ex.getMessage());
+ return parser.parseFrom(value.toByteArray());
Review Comment:
@Russole , Just find that we should asReadOnlyByteBuffer() here.
```java
return parser.parseFrom(value.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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "Russole (via GitHub)" <gi...@apache.org>.
Russole commented on PR #9878:
URL: https://github.com/apache/ozone/pull/9878#issuecomment-4017767292
Thanks @szetszwo for pointing that out. I've updated the patch accordingly.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9878:
URL: https://github.com/apache/ozone/pull/9878#discussion_r2898984099
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmNonShadedGeneratedMessageCodec.java:
##########
@@ -28,25 +27,25 @@
* {@link ScmCodec} implementation for non-shaded
* {@link com.google.protobuf.Message} objects.
*/
-public class ScmNonShadedGeneratedMessageCodec implements ScmCodec<Object> {
+public class ScmNonShadedGeneratedMessageCodec<T extends Message> implements ScmCodec<T> {
+
+ private final Parser<T> parser;
+
+ public ScmNonShadedGeneratedMessageCodec(Parser<T> parser) {
+ this.parser = parser;
+ }
@Override
- public ByteString serialize(Object object)
- throws InvalidProtocolBufferException {
- return UnsafeByteOperations.unsafeWrap(
- ((Message) object).toByteString().asReadOnlyByteBuffer());
+ public ByteString serialize(T object) throws InvalidProtocolBufferException {
+ return UnsafeByteOperations.unsafeWrap(object.toByteString().asReadOnlyByteBuffer());
}
@Override
- public Object deserialize(Class<?> type, ByteString value)
- throws InvalidProtocolBufferException {
+ public T deserialize(Class<?> type, ByteString value) throws InvalidProtocolBufferException {
try {
- return ReflectionUtil.getMethod(type, "parseFrom", byte[].class)
- .invoke(null, (Object) value.toByteArray());
- } catch (NoSuchMethodException | IllegalAccessException
- | InvocationTargetException ex) {
- ex.printStackTrace();
- throw new InvalidProtocolBufferException("Message cannot be decoded: " + ex.getMessage());
+ return parser.parseFrom(value.toByteArray());
+ } catch (com.google.protobuf.InvalidProtocolBufferException e) {
+ throw new IllegalArgumentException("Message cannot be decoded", e);
Review Comment:
Sorry that this comment somehow was not included preiously.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "Russole (via GitHub)" <gi...@apache.org>.
Russole commented on PR #9878:
URL: https://github.com/apache/ozone/pull/9878#issuecomment-4015550026
Thanks @szetszwo for the review!
I’m happy to update the change based on any comments.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #9878:
URL: https://github.com/apache/ozone/pull/9878#issuecomment-4018662803
Thanks @Russole for the patch, @szetszwo for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "Russole (via GitHub)" <gi...@apache.org>.
Russole commented on PR #9878:
URL: https://github.com/apache/ozone/pull/9878#issuecomment-4015112931
Hi @szetszwo, when you have time, could you please help review this PR? Thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #9878:
URL: https://github.com/apache/ozone/pull/9878#discussion_r2899477134
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java:
##########
@@ -42,8 +47,18 @@ public final class ScmCodecFactory {
private static Map<Class<?>, ScmCodec<?>> codecs = new HashMap<>();
static {
- codecs.put(com.google.protobuf.Message.class, new ScmNonShadedGeneratedMessageCodec());
- codecs.put(Message.class, new ScmGeneratedMessageCodec());
+ codecs.put(ContainerID.class,
+ new ScmNonShadedGeneratedMessageCodec<>(ContainerID.parser()));
+ codecs.put(PipelineID.class,
+ new ScmNonShadedGeneratedMessageCodec<>(PipelineID.parser()));
+ codecs.put(Pipeline.class,
+ new ScmNonShadedGeneratedMessageCodec<>(Pipeline.parser()));
+ codecs.put(ContainerInfoProto.class,
+ new ScmNonShadedGeneratedMessageCodec<>(ContainerInfoProto.parser()));
+ codecs.put(DeletedBlocksTransaction.class,
+ new ScmNonShadedGeneratedMessageCodec<>(DeletedBlocksTransaction.parser()));
+ codecs.put(DeletedBlocksTransactionSummary.class,
+ new ScmNonShadedGeneratedMessageCodec<>(DeletedBlocksTransactionSummary.parser()));
Review Comment:
@Russole , I forgot to say we may use the getDefaultInstance() trick;
```java
putProto(ContainerID.getDefaultInstance());
putProto(PipelineID.getDefaultInstance());
putProto(Pipeline.getDefaultInstance());
putProto(ContainerInfoProto.getDefaultInstance());
putProto(DeletedBlocksTransaction.getDefaultInstance());
putProto(DeletedBlocksTransactionSummary.getDefaultInstance());
```
```java
static <T extends Message> void putProto(T proto) {
codecs.put(proto.getClass(), new ScmNonShadedGeneratedMessageCodec<>(proto.getParserForType()));
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-14748. Implement ScmGeneratedMessageCodec without reflection [ozone]
Posted by "Russole (via GitHub)" <gi...@apache.org>.
Russole commented on PR #9878:
URL: https://github.com/apache/ozone/pull/9878#issuecomment-4016928201
Thanks @szetszwo for the suggestion. I've updated the implementation accordingly.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org