adoroszlai commented on code in PR #6911:
URL: https://github.com/apache/ozone/pull/6911#discussion_r1677390214
##########
hadoop-ozone/dist/src/main/smoketest/balancer/testBalancer.robot:
##########
@@ -63,6 +63,22 @@ Datanode Recommission is Finished
Run Container Balancer
${result} = Execute ozone admin
containerbalancer start -t 1 -d 100 -i 1
Should Contain ${result}
Container Balancer started successfully.
+ ${verboseStatusResult} = Execute ozone admin
containerbalancer status -v
+ Should Contain
${verboseStatusResult} ContainerBalancer is Running.
+ Should Contain
${verboseStatusResult} Started at:
+ Should Contain
${verboseStatusResult} Container Balancer Configuration values:
+ Should Contain
${verboseStatusResult} Current iteration info:
+ Should Contain
${verboseStatusResult} Iteration number
1
+ Should Contain
${verboseStatusResult} Iteration result
IN_PROGRESS
+ Should Contain
${verboseStatusResult} Scheduled to move containers
3
+ ${statusHistoryResult} = Execute ozone admin
containerbalancer status -v -h
+ Should Contain
${statusHistoryResult} ContainerBalancer is Running.
+ Should Contain
${statusHistoryResult} Started at:
+ Should Contain
${statusHistoryResult} Container Balancer Configuration values:
+ Should Contain
${statusHistoryResult} Iteration history list:
+ Should Contain
${statusHistoryResult} Iteration number
1
+ Should Contain
${statusHistoryResult} Iteration result
IN_PROGRESS
+ Should Contain
${statusHistoryResult} Scheduled to move containers
3
Review Comment:
> Please create a reusable keyword to check expected output.
Sorry, if my comment could be misinterpreted.
By "reusable keyword" I meant something like this:
```robot
Verify Balancer Status
[arguments] ${output}
Should Contain ${output} ContainerBalancer is Running.
Should Contain ${output} Started at:
Should Contain ${output} Container Balancer Configuration values:
Verify Balancer Iteration
[arguments] ${output} ${number} ${status} ${containers}
Should Contain ${output} Iteration number
${number}
Should Contain ${output} Iteration result
${status}
Should Contain ${output} Scheduled to move containers
${containers}
```
Then the main test could be shortened:
```robot
${result} = Execute ozone admin containerbalancer status -v
Verify Balancer Status Output ${result}
Verify Balancer Iteration ${result} 1 IN_PROGRESS 3
Should Contain ${result} Current iteration info:
${result} = Execute ozone admin containerbalancer status -v
--history
Verify Balancer Status Output ${result}
Verify Balancer Iteration ${result} 1 IN_PROGRESS 3
Should Contain ${result} Iteration history list:
```
We could add a keyword for checking output with history and another for the
plain verbose output. They could verify that one the right one of "Current
iteration info" and "Iteration history list" is present, the other one is not.
Please feel free to change this example as needed.
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java:
##########
@@ -33,13 +45,133 @@
versionProvider = HddsVersionProvider.class)
public class ContainerBalancerStatusSubcommand extends ScmSubcommand {
+ @CommandLine.Option(names = {"-v", "--verbose"},
+ description = "Verbose output. Show current iteration info.")
+ private boolean verbose;
+
+ @CommandLine.Option(names = {"-h", "--history"},
Review Comment:
`-h` is for `--help`, which the command gets for free via
`mixinStandardHelpOptions = true`.
https://picocli.info/#_mixin_standard_help_options
I suggested `-H` for this reason.
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java:
##########
@@ -33,13 +45,133 @@
versionProvider = HddsVersionProvider.class)
public class ContainerBalancerStatusSubcommand extends ScmSubcommand {
+ @CommandLine.Option(names = {"-v", "--verbose"},
+ description = "Verbose output. Show current iteration info.")
+ private boolean verbose;
+
+ @CommandLine.Option(names = {"-h", "--history"},
+ description = "Verbose output with history. Show current iteration info
and history of iterations. " +
+ "Works only with -v.")
+ private boolean verboseWithHistory;
+
@Override
public void execute(ScmClient scmClient) throws IOException {
- boolean execReturn = scmClient.getContainerBalancerStatus();
- if (execReturn) {
+ ContainerBalancerStatusInfoResponseProto response =
scmClient.getContainerBalancerStatusInfo();
+ boolean isRunning = response.getIsRunning();
+ ContainerBalancerStatusInfo balancerStatusInfo =
response.getContainerBalancerStatusInfo();
+ if (isRunning) {
+ LocalDateTime dateTime =
+
LocalDateTime.ofInstant(Instant.ofEpochSecond(balancerStatusInfo.getStartedAt()),
ZoneId.systemDefault());
System.out.println("ContainerBalancer is Running.");
+
+ if (verbose) {
+ System.out.printf("Started at: %s %s%n%n", dateTime.toLocalDate(),
dateTime.toLocalTime());
+
System.out.println(getConfigurationPrettyString(balancerStatusInfo.getConfiguration()));
+ List<ContainerBalancerTaskIterationStatusInfo> iterationsStatusInfoList
+ = balancerStatusInfo.getIterationsStatusInfoList();
+ if (!verboseWithHistory) {
+ System.out.println("Current iteration info:");
+ System.out.println(
+
getPrettyIterationStatusInfo(iterationsStatusInfoList.get(iterationsStatusInfoList.size()
- 1))
+ );
+ } else if (verboseWithHistory) {
+ System.out.println("Iteration history list:");
+ System.out.println(
+
iterationsStatusInfoList.stream().map(this::getPrettyIterationStatusInfo)
+ .collect(Collectors.joining("\n"))
+ );
Review Comment:
nit: no need for using `else if` with boolean, it must be true or false.
```
if (verboseWithHistory) {
...
} else {
...
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
