adoroszlai commented on code in PR #6911:
URL: https://github.com/apache/ozone/pull/6911#discussion_r1676777350
##########
hadoop-hdds/interface-admin/src/main/resources/proto.lock:
##########
Review Comment:
Please do not change `proto.lock`. It serves as a reference to check for
compatibility problems.
##########
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.
+ ${result} = Execute ozone admin
containerbalancer status -v
+ Should Contain ${result}
ContainerBalancer is Running.
+ Should Contain ${result}
Started at:
+ Should Contain ${result}
Container Balancer Configuration values:
+ Should Contain ${result}
Current iteration info:
+ Should Contain ${result}
Iteration number 1
+ Should Contain ${result}
Iteration result IN_PROGRESS
+ Should Contain ${result}
Scheduled to move containers 3
Review Comment:
Please create a reusable keyword to check expected output.
##########
hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto:
##########
@@ -607,6 +610,38 @@ message ContainerBalancerStatusResponseProto {
required bool isRunning = 1;
}
+message ContainerBalancerStatusInfoRequestProto {
+ optional string traceID = 1;
+}
+
+message ContainerBalancerStatusInfoResponseProto {
+ required bool isRunning = 1;
Review Comment:
Adding all new fields as `optional` is recommended:
https://protobuf.dev/programming-guides/dos-donts/#add-required
##########
hadoop-hdds/tools/pom.xml:
##########
@@ -109,5 +109,11 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd";>
<artifactId>hdds-test-utils</artifactId>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>org.apache.ozone</groupId>
+ <artifactId>hdds-interface-admin</artifactId>
+ <version>${hdds.version}</version>
+ </dependency>
Review Comment:
`hdds-interface-admin` is already a transitive dependency via
`hdds-server-framework`, so I don't think this is needed.
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java:
##########
@@ -33,13 +45,132 @@
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 = {"-vh", "--verbose-with-history"},
Review Comment:
Please do not add multi-char short options (`-vh`).
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java:
##########
@@ -33,13 +45,132 @@
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 = {"-vh", "--verbose-with-history"},
+ description = "More verbose output. Show current and history iteration
info.")
+ private boolean verboseWithHistory;
Review Comment:
This option would be better as `-H, --history`, and handled as an extra for
when verbose output is enabled by `--verbose`. It can be ignored when
`verbose==false`. This simplifies logic both for users and for implementation
(no need for `verbose || verboseWithHistory`).
--
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]
