HDDS-14846. Implement ScmCodecFactory without reflection by Russole · Pull Request #9952 · apache/ozone

Russole · 2026-03-19T23:04:36Z

What changes were proposed in this pull request?

Replace Class parameter in ScmCodecFactory.getCodec with class name (String)
Move type resolution handling from getCodec to ClassResolver
Remove superclass/interface scanning and remove reflection-based lookup
Update all call sites to switch to class name for codec resolution
Refactor ScmCodecFactory into a singleton (Convert codec map to non-static and add a static instance)

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14846

How was this patch tested?

All CI checks passed
- https://github.com/Russole/ozone/actions/runs/23306340711

Russole · 2026-03-19T23:05:37Z

Hi @szetszwo, could you please help review this PR when you have time? Thanks!

szetszwo · 2026-03-20T18:14:01Z

@Russole , When reviewing this, I found that the change can be simplified after remove the type parameter from ScmCodec.deserialize(..); filed HDDS-14875.

szetszwo

@Russole , thanks for working on this! Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13081315/9952_review.patch

BTW, please wait for HDDS-14875 -- 9952_review.patch requires it.

szetszwo · 2026-03-20T17:38:03Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java

+      final String parameterType = parameterTypes[paramCounter++].getName();
+      argBuilder.setType(parameterType);
+      argBuilder.setValue(ScmCodecFactory.getInstance().getCodec(parameterType)


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.

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()); }

szetszwo · 2026-03-20T17:38:57Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisRequest.java

+        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)


Use factory to resolve the class:

final String argumentType = argument.getType(); final Class<?> clazz = FACTORY.resolve(argumentType); parameterTypes[paramCounter++] = clazz; args.add(FACTORY.getCodec(clazz).deserialize(argument.getValue()));

szetszwo · 2026-03-20T17:42:57Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisResponse.java

    final Class<?> type = result.getClass();
+    final String typeName = type.getName();


Pass the declaration type as a parameter:

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())); }

szetszwo · 2026-03-20T18:26:46Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisResponse.java

-      final Class<?> type = ReflectionUtil.getClass(responseProto.getType());
-      return new SCMRatisResponse(ScmCodecFactory.getCodec(type)
+      final String typeName = responseProto.getType();
+      final Class<?> type = ReflectionUtil.getClass(typeName);


We should use the factory to resolve it.

final Class<?> type = FACTORY.resolve(responseProto.getType()); return new SCMRatisResponse(FACTORY.getCodec(type).deserialize(responseProto.getValue()));

szetszwo · 2026-03-20T18:30:55Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java

-  public static ScmCodec getCodec(Class<?> type)
+  public ScmCodec getCodec(String className)


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/io/ScmCodecFactory.java

Russole · 2026-03-21T09:15:00Z

Thanks @szetszwo for the review. I will update the patch after HDDS-14875 is merged.

szetszwo

+1 the change looks good.

adoroszlai · 2026-03-22T07:31:17Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java

-    classes.addAll(ClassUtils.getAllSuperclasses(type));
-    classes.addAll(ClassUtils.getAllInterfaces(type));


I think removing supertypes causes the following error:

org.apache.hadoop.hdds.scm.exceptions.SCMException: org.apache.ratis.thirdparty.com.google.protobuf.InvalidProtocolBufferException: Codec not found for class java.util.ArrayList at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.translateException(SCMHAInvocationHandler.java:163) at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.invokeRatis(SCMHAInvocationHandler.java:113) at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.invoke(SCMHAInvocationHandler.java:72) at jdk.proxy2/jdk.proxy2.$Proxy41.addTransactionsToDB(Unknown Source) at org.apache.hadoop.hdds.scm.block.SCMDeletedBlockTransactionStatusManager.addTransactions(SCMDeletedBlockTransactionStatusManager.java:470) at org.apache.hadoop.hdds.scm.block.DeletedBlockLogImpl.addTransactions(DeletedBlockLogImpl.java:253) at org.apache.hadoop.ozone.shell.TestDeletedBlocksTxnShell.testGetDeletedBlockSummarySubcommand(TestDeletedBlocksTxnShell.java:178) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387) at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1312) at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1843) at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1808) at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:188) Caused by: org.apache.ratis.thirdparty.com.google.protobuf.InvalidProtocolBufferException: Codec not found for class java.util.ArrayList at org.apache.hadoop.hdds.scm.ha.io.ScmCodecFactory.getCodec(ScmCodecFactory.java:107) at org.apache.hadoop.hdds.scm.ha.SCMRatisRequest.encode(SCMRatisRequest.java:107) at org.apache.hadoop.hdds.scm.ha.SCMRatisServerImpl.submitRequest(SCMRatisServerImpl.java:238) at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.invokeRatisServer(SCMHAInvocationHandler.java:121) at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.invokeRatis(SCMHAInvocationHandler.java:110)

Maybe we can fix it by changing ArrayList parameter to List in DeletedBlockLogStateManager and its implementation:

ozone/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogStateManager.java

Lines 32 to 56 in 6cdc9fc

public interface DeletedBlockLogStateManager {

@Replicate

void addTransactionsToDB(ArrayList<DeletedBlocksTransaction> txs,

DeletedBlocksTransactionSummary summary) throws IOException;

@Replicate

void addTransactionsToDB(ArrayList<DeletedBlocksTransaction> txs) throws IOException;

@Replicate

void removeTransactionsFromDB(ArrayList<Long> txIDs, DeletedBlocksTransactionSummary summary)

throws IOException;

@Replicate

void removeTransactionsFromDB(ArrayList<Long> txIDs)

throws IOException;

@Deprecated

@Replicate

void increaseRetryCountOfTransactionInDB(ArrayList<Long> txIDs)

throws IOException;

@Deprecated

@Replicate

int resetRetryCountOfTransactionInDB(ArrayList<Long> txIDs)

throws IOException;

Declaring as List is good practice in any case.

Russole/ozone@HDDS-14846...adoroszlai:ozone:HDDS-14846

@adoroszlai , thanks for debugging the test failures!

We should put ArrayList to the factory for backward compatibility.

+++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java @@ -75,7 +75,9 @@ public final class ScmCodecFactory { // Must be the last one final ClassResolver resolver = new ClassResolver(codecs.keySet()); - codecs.put(List.class, new ScmListCodec(resolver)); + final ScmListCodec listCodec = new ScmListCodec(resolver); + codecs.put(List.class, listCodec); + codecs.put(ArrayList.class, listCodec); }

Russole · 2026-03-22T23:12:17Z

Thanks @adoroszlai and @szetszwo for the suggestion.

I think the issue is that the method parameter types and the codec-resolved types were previously treated as the same, which can lead to method lookup failures during decoding and codec lookup failures during encoding.

So I separated the two concerns:

use the original declared type (via getClass) for method signature reconstruction
use the resolved type for codec serialization/deserialization

I also added a getClass(String) helper in ScmCodecFactory to reconstruct the declared parameter types from the serialized type names:

  public Class<?> getClass(String className)
      throws InvalidProtocolBufferException {
    try {
      return Class.forName(className);
    } catch (ClassNotFoundException e) {
      throw new InvalidProtocolBufferException(
          "Class not found for " + className, e);
    }
  }

CI tests have passed: https://github.com/Russole/ozone/actions/runs/23406070564

szetszwo

@Russole , thanks for the update!

Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13081331/9952_review2.patch

szetszwo · 2026-03-23T03:01:54Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java

+    throw new InvalidProtocolBufferException("Codec not found for " + resolved);
+  }
+
+  public Class<?> getClass(String className)


Add one more map to avoid calling Class.forName(..) multiple times for the same class.

public Class<?> getClass(String className) throws InvalidProtocolBufferException { final Class<?> found = classes.get(className); if (found != null) { return found; } final Class<?> clazz; try { clazz = Class.forName(className); } catch (ClassNotFoundException e) { throw new InvalidProtocolBufferException("Class not found for " + className, e); } classes.put(className, clazz); return clazz; }

szetszwo · 2026-03-23T03:10:59Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java

+        try {
+          clazz = Class.forName(className);
+        } catch (ClassNotFoundException e) {
+          throw new InvalidProtocolBufferException("Class not found for " + className, e);
+        }


Reuse the getClass method

clazz = getInstance().getClass(className);

szetszwo · 2026-03-23T03:12:26Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/io/ScmCodecFactory.java

+  }

-  public static ScmCodec getCodec(Class<?> type)
+  public Class<?> resolve(String className)


Add one more resolve method for Class<?>.

public Class<?> resolve(Class<?> clazz) throws InvalidProtocolBufferException { return resolver.get(clazz); }

szetszwo

+1 the change looks good.

Russole added 2 commits March 19, 2026 22:15

HDDS-14846. Implement ScmCodecFactory without reflection.

3afefdc

Refactor ScmCodecFactory to singleton

72446ed

Russole changed the title ~~HDDS-14846.~~ HDDS-14846. Implement ScmCodecFactory without reflection. Mar 19, 2026

Russole changed the title ~~HDDS-14846. Implement ScmCodecFactory without reflection.~~ HDDS-14846. Implement ScmCodecFactory without reflection Mar 19, 2026

adoroszlai requested a review from szetszwo March 20, 2026 07:21

szetszwo reviewed Mar 20, 2026

View reviewed changes

Russole added 3 commits March 22, 2026 08:26

Merge branch 'master' into HDDS-14846

df0ef7c

Update change based on review comments

8ccb23d

Change ClassResolver.get helper method to private

e45787a

szetszwo approved these changes Mar 22, 2026

View reviewed changes

adoroszlai reviewed Mar 22, 2026

View reviewed changes

Fix parameter type handling in SCMRatisRequest decode

65b853e

Russole requested a review from szetszwo March 23, 2026 02:17

szetszwo reviewed Mar 23, 2026

View reviewed changes

Update patch based on review comments

7808120

Russole requested a review from szetszwo March 23, 2026 17:28

szetszwo approved these changes Mar 23, 2026

View reviewed changes

szetszwo merged commit 9c97ef2 into apache:master Mar 23, 2026
45 checks passed

		final Class<?> type = result.getClass();
		final String typeName = type.getName();

		public static ScmCodec getCodec(Class<?> type)
		public ScmCodec getCodec(String className)

		classes.addAll(ClassUtils.getAllSuperclasses(type));
		classes.addAll(ClassUtils.getAllInterfaces(type));

	public interface DeletedBlockLogStateManager {
	@Replicate
	void addTransactionsToDB(ArrayList<DeletedBlocksTransaction> txs,
	DeletedBlocksTransactionSummary summary) throws IOException;

	@Replicate
	void addTransactionsToDB(ArrayList<DeletedBlocksTransaction> txs) throws IOException;

	@Replicate
	void removeTransactionsFromDB(ArrayList<Long> txIDs, DeletedBlocksTransactionSummary summary)
	throws IOException;

	@Replicate
	void removeTransactionsFromDB(ArrayList<Long> txIDs)
	throws IOException;

	@Deprecated
	@Replicate
	void increaseRetryCountOfTransactionInDB(ArrayList<Long> txIDs)
	throws IOException;

	@Deprecated
	@Replicate
	int resetRetryCountOfTransactionInDB(ArrayList<Long> txIDs)
	throws IOException;

Conversation

Russole commented Mar 19, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Russole commented Mar 19, 2026

Uh oh!

szetszwo commented Mar 20, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Russole commented Mar 21, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Russole commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adoroszlai Mar 22, 2026 •

edited

Loading

szetszwo Mar 22, 2026 •

edited

Loading

Russole commented Mar 22, 2026 •

edited

Loading