HDDS-7234. Add a common option for DiskBalancer commands #3762
Conversation
|
@lokeshj1703 Please help to review. |
dineshchitlangia
left a comment
dineshchitlangia
left a comment
There was a problem hiding this comment.
Overall looks good, suggested minor changes.
Thank you for your contribution.
...s/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerCommonOptions.java
Outdated
Show resolved
Hide resolved
...s/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerCommonOptions.java
Outdated
Show resolved
Hide resolved
| @CommandLine.Option(names = {"-a", "--allDatanodes"}, | ||
| description = "Run commands on all datanodes.") |
There was a problem hiding this comment.
Consider replacing --allDatanodes with just --all
| @CommandLine.Option(names = {"--hosts"}, | ||
| description = "Run commands on specific datanodes.") |
There was a problem hiding this comment.
To maintain consistency with other options, we should have both short and a long option name -> -h and --hosts
If -h is used for help or conflicts with any existing options, consider using -d and --datanodes
…/datanode/DiskBalancerCommonOptions.java Co-authored-by: Dinesh Chitlangia <dineshchitlangia@gmail.com>
…/datanode/DiskBalancerCommonOptions.java Co-authored-by: Dinesh Chitlangia <dineshchitlangia@gmail.com>
lokeshj1703
left a comment
lokeshj1703
left a comment
There was a problem hiding this comment.
@symious Thanks for working on this! The changes look good to me. I have a few comments inline.
...s/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerCommonOptions.java
Outdated
Show resolved
Hide resolved
...tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStartSubcommand.java
Outdated
Show resolved
Hide resolved
...tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStartSubcommand.java
Outdated
Show resolved
Hide resolved
|
@dineshchitlangia @lokeshj1703 Updated the PR, please have a look. |
|
@dineshchitlangia @lokeshj1703 Hi, do you have other comments? Plan to merge it tmr. |
lokeshj1703
left a comment
lokeshj1703
left a comment
There was a problem hiding this comment.
Thanks @symious for updating the PR! The changes look good to me. +1.
|
@symious Thanks for your contribution! @dineshchitlangia @lokeshj1703 Thanks for your careful review! Merged |
|
@ferhui @lokeshj1703 @dineshchitlangia Thank you for the review. |
4 participants
What changes were proposed in this pull request?
This ticket is to add a common option for DiskBalancer CLI.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7234
How was this patch tested?
original unit test.