Skip to content

HDDS-15190. Add ScmInvoker subclasses for CertificateStore and RootCARotationHandler#10226

Merged
adoroszlai merged 7 commits into
apache:masterfrom
YutaLin:HDDS-15190_Add_ScmInvoker_subclasses_for_CertificateStore_and_RootCARotationHandler
May 13, 2026
Merged

HDDS-15190. Add ScmInvoker subclasses for CertificateStore and RootCARotationHandler#10226
adoroszlai merged 7 commits into
apache:masterfrom
YutaLin:HDDS-15190_Add_ScmInvoker_subclasses_for_CertificateStore_and_RootCARotationHandler

Conversation

@YutaLin
Copy link
Copy Markdown
Contributor

@YutaLin YutaLin commented May 10, 2026

What changes were proposed in this pull request?

CertificateStore and RootCARotationHandler have Replicate.InvocationType.CLIENT methods. We need to add a new method ScmInvoker.invokeReplicateClient(..), which is similar to SCMHAInvocationHandler.invokeRatisClient(..).

  • Add CertificationStoreInvoker, RootCARotationHandlerInvoker
  • Use scminvoker certifcate store and root CA rotation instead
  • Update TestSCMCertStore

What is the link to the Apache JIRA

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

How was this patch tested?

CI (https://github.com/YutaLin/ozone/actions/runs/25616356321)

Copy link
Copy Markdown
Contributor

@Russole Russole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @YutaLin for the patch. Overall LGTM.

One small suggestion: RootCARotationHandlerInvoker is marked as generated code, but the method order in newProxy() differs from ScmInvokerCodeGenerator output. It would be better to keep the generated order for consistency with the other ScmInvoker implementations.

I generated the equivalent change here:
Russole@b0decc7

This is not a functional change, only method ordering plus the generator main method in RootCARotationHandler.

@YutaLin
Copy link
Copy Markdown
Contributor Author

YutaLin commented May 10, 2026

Thanks @Russole for the review!

@Russole
Copy link
Copy Markdown
Contributor

Russole commented May 11, 2026

Thanks @YutaLin. Could you also apply this small consistency update before merge?

Since RootCARotationHandlerInvoker is marked as generated code, I think it is better to keep it aligned with ScmInvokerCodeGenerator output.

@adoroszlai
Copy link
Copy Markdown
Contributor

adoroszlai commented May 11, 2026

One small suggestion: RootCARotationHandlerInvoker is marked as generated code, but the method order in newProxy() differs from ScmInvokerCodeGenerator output. It would be better to keep the generated order for consistency with the other ScmInvoker implementations.

Agreed.

@Russole if you ask for changes please don't trigger CI, since it will have to be re-run after patch is updated

@Russole
Copy link
Copy Markdown
Contributor

Russole commented May 11, 2026

Thanks @adoroszlai, noted. I’ll avoid triggering CI when requesting changes.

@YutaLin
Copy link
Copy Markdown
Contributor Author

YutaLin commented May 12, 2026

HI @Russole @adoroszlai
Thanks for the review, I've made some change.
FYI, I don't add ScmInvokerCodeGenerator.generate in CertificateStore because it's in different module, and it will have circular dependency If i import the module. It's also not a big problem since we only use it to generate Invoker.

@YutaLin YutaLin requested a review from Russole May 12, 2026 04:07
Copy link
Copy Markdown
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 the change looks good.

Copy link
Copy Markdown
Contributor

@Russole Russole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @YutaLin for working on this. LGTM!

@adoroszlai adoroszlai merged commit b94fcbc into apache:master May 13, 2026
90 of 91 checks passed
@adoroszlai
Copy link
Copy Markdown
Contributor

Thanks @YutaLin for the patch, @Russole, @szetszwo for the review.

@YutaLin YutaLin deleted the HDDS-15190_Add_ScmInvoker_subclasses_for_CertificateStore_and_RootCARotationHandler branch May 13, 2026 17:35
@YutaLin
Copy link
Copy Markdown
Contributor Author

YutaLin commented May 13, 2026

Thanks @adoroszlai for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants