Copilot commented on code in PR #10022:
URL: https://github.com/apache/ozone/pull/10022#discussion_r3115391005
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStartSubcommand.java:
##########
@@ -58,6 +58,11 @@ public class DiskBalancerStartSubcommand extends
AbstractDiskBalancerSubCommand
arity = "1")
private Boolean stopAfterDiskEven;
+ @Option(names = {"--c", "--container-states"},
Review Comment:
The start subcommand defines the short option as "--c" (a long option named
"c") instead of the conventional "-c". This makes the CLI inconsistent with the
update subcommand (which uses "-c") and will likely surprise users. Consider
changing the option names to {"-c", "--container-states"} to match the other
subcommand.
```suggestion
@Option(names = {"-c", "--container-states"},
```
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStatusSubcommand.java:
##########
@@ -135,7 +139,9 @@ private String
generateStatus(List<DatanodeDiskBalancerInfoProto> protos) {
.append(" - EstBytesToMove is calculated based on the target disk even
state with the configured threshold.%n")
.append(" - EstTimeLeft is calculated based on EstimatedBytesToMove
and configured disk bandwidth.%n")
.append(" - Both EstimatedBytes and EstTimeLeft could be non-zero
while no containers" +
- " can be moved, especially when the configured threshold or disk
capacity is too small.");
+ " can be moved, especially when the configured threshold or disk
capacity is too small.%n")
+ .append(" - ContainerStates indicate the state of container which is
eligible for balancing," +
+ " by default CLOSED and QUASI_CLOSED are allowed.");
Review Comment:
Minor grammar: "ContainerStates indicate the state of container which is
eligible" reads awkwardly. Consider rephrasing to something like
"ContainerStates indicates which container lifecycle states are eligible for
balancing" (and keep subject/verb agreement consistent).
```suggestion
.append(" - ContainerStates indicates which container lifecycle
states are eligible for balancing;" +
" by default, CLOSED and QUASI_CLOSED are allowed.");
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerConfiguration.java:
##########
@@ -37,6 +44,11 @@ public final class DiskBalancerConfiguration {
private static final Logger LOG =
LoggerFactory.getLogger(DiskBalancerConfiguration.class);
+ /**
+ * Default {@link #containerStates}: CLOSED and QUASI_CLOSED containers may
be disk-balanced.
+ */
+ public static final String DEFAULT_CONTAINER_STATES = "CLOSED,QUASI_CLOSED";
+
Review Comment:
This PR description mentions a boolean config (include non-standard
containers) defaulting to false, but the code here introduces a different
config (hdds.datanode.disk.balancer.container.states) with a default that
already includes QUASI_CLOSED. Please confirm the intended user-facing behavior
and ensure the PR description / release notes and defaults align (especially if
this changes the default balancing behavior).
--
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]
