-
Notifications
You must be signed in to change notification settings - Fork 414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RATIS-2095. Extract common logic of ratis-shell to RaftUtils for reuse #1098
Conversation
cc. @szetszwo |
For reference, the reason I can't use ratis's AbstractRatisCommand and it's suite of subcommands in Ozone's shell directly is because Ozone shell uses picocli framework, and Ratis shell uses apache.commons.cli.CommandLine framework. |
Thanks @szetszwo suggestion, I'll separate the security related features (changes of ratis-shell/src/main/java/org/apache/ratis/shell/cli/SecurityUtils.java) to another jira/PR later. I think for now Ozone shell doesn't need that part of security utility feature from the ratis side since Ozone has its own way of getting client's certificate and build ratis client on ozone's side and then pass client to methods in new utility class (ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java) to call ratis server. |
Also, I'll revert changes in those subcommands of AbstractRatisCommand and keep original processReply method in AbstractRatisCommand. |
…tRatisCommand to use original processReply method of AbstractRatisCommand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaveTeng0 , thanks for the update! Please see the comments inlined.
BTW, please feel free to ping me when this pull request is ready. Otherwise, I cannot tell.
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
Outdated
Show resolved
Hide resolved
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
Outdated
Show resolved
Hide resolved
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
Outdated
Show resolved
Hide resolved
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
Outdated
Show resolved
Hide resolved
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
Outdated
Show resolved
Hide resolved
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
Outdated
Show resolved
Hide resolved
ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/command/AbstractRatisCommand.java
Outdated
Show resolved
Hide resolved
ratis-shell/src/main/java/org/apache/ratis/shell/cli/RaftUtils.java
Outdated
Show resolved
Hide resolved
Hello guys! Changes for addressing comments have been completed. Feel free to take a final look, thanks! (cc. @szetszwo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaveTeng0 , thanks for the update! There is a checkstyle issue. I also response and un-resolve to some previous comments.
There was a problem hiding this 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.
Since RaftUtils
is going to be a public API, I do have some more suggestions:
- Use consistent method names:
getPeerId
vsbuildRaftPeersFromStr
. - Use consistent parameter type:
PrintStream
vsConsumer<String>
- Remove duplicated
AbstractCommand.parseInetSocketAddress
Let's do them separately.
What changes were proposed in this pull request?
We have an Ozone Jira that we'd like to run ratis command from Ozone shell (https://issues.apache.org/jira/browse/HDDS-10509).
In order to share the common logic between ratis-shell and ozone-shell,
move those common logic into RaftUtils so that Ozone shell could import and reuse those methods.
(related:
HDDS-10891 Support Ozone to run ratis group-related command.
draft PR: apache/ozone#6716 (This is pending by this RATIS-2095 PR to be merged first, so that Ozone can use those shared methods from ratis-shell.jar)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2095
How was this patch tested?
Original unit tests suite