ArafatKhan2198 commented on code in PR #10681:
URL: https://github.com/apache/ozone/pull/10681#discussion_r3541695912
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java:
##########
@@ -397,6 +412,7 @@ public void stopBalancer()
saveConfiguration(config, false, 0);
if (isBalancerRunning()) {
LOG.info("Trying to stop ContainerBalancer service.");
+ task.recordStopReason("STOPPED_BY_USER");
Review Comment:
**Stop reasons are free‑form strings with inconsistent formatting and
duplicated literals.** They mix `UPPER_SNAKE_CASE` and sentences:
- `ContainerBalancer.stop()` → `"STOPPED_ON_SCM_STATE_CHANGE"`,
`stopBalancer()` → `"STOPPED_BY_USER"`
- `ContainerBalancerTask`: `"Completed all iterations."`,
`IterationResult.CAN_NOT_BALANCE_ANY_MORE.toString()`, `"STOPPED"`,
`"STOPPED_WITH_ERROR: " + e.getMessage()`, `"Could not initialize
ContainerBalancer's iteration number " + i`
This is user‑visible output. The literals also live across two classes with
no shared definition. Recommend a small `StopReason` enum (or shared constants)
with a display string, so the reasons are consistent, discoverable, and
testable.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java:
##########
@@ -186,13 +186,27 @@ public ContainerBalancerTask.Status getBalancerStatus() {
public ContainerBalancerStatusInfo getBalancerStatusInfo() throws
IOException {
lock.lock();
try {
- if (isBalancerRunning()) {
+ if (task == null) {
+ return null;
+ }
+ ContainerBalancerTask.Status status = task.getBalancerStatus();
+ if (status == ContainerBalancerTask.Status.RUNNING
+ || status == ContainerBalancerTask.Status.STOPPING) {
Review Comment:
STOPPING window shows no reason, the balancer has three states: RUNNING →
STOPPING → STOPPED. STOPPING is the brief "I've been told to stop and I'm
winding down" phase.
So for that short window the CLI prints "Not Running" with no reason, even
though the reason exists.
Not much of a deal tho because STOPPING is a transient state that usually
lasts a moment, so most people never catch it. But it's a real gap.
You can add it if you want it's up to you. It should be fine without it as
well.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -190,8 +192,16 @@ public void run() {
balance();
} catch (Exception e) {
LOG.error("Container Balancer is stopped abnormally, ", e);
+ recordStopReason("STOPPED_WITH_ERROR: " + e.getMessage());
Review Comment:
`"STOPPED_WITH_ERROR: " + e.getMessage()` → yields `"...: null"` when the
exception has no message. Use `e.toString()` or guard.
##########
hadoop-hdds/interface-admin/src/main/resources/proto.lock:
##########
@@ -2514,4 +2526,4 @@
}
}
]
-}
\ No newline at end of file
+}
Review Comment:
No need for this.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -190,8 +192,16 @@ public void run() {
balance();
} catch (Exception e) {
LOG.error("Container Balancer is stopped abnormally, ", e);
+ recordStopReason("STOPPED_WITH_ERROR: " + e.getMessage());
Review Comment:
Do correct me If I am missing something.
**The `STOPPED_WITH_ERROR` / exception path is untested.** The new code in
`run()`'s `finally` (`recordStopReason("STOPPED_WITH_ERROR: " +
e.getMessage())` + `finalizeInProgressIteration`) only fires when `balance()`
throws. No test forces an exception, so that whole branch plus the `finalize`
synthesizing an interrupted iteration on a crash isn't tested.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java:
##########
@@ -60,50 +60,84 @@ public void execute(ScmClient scmClient) throws IOException
{
boolean isRunning = response.getIsRunning();
ContainerBalancerStatusInfoProto balancerStatusInfo =
response.getContainerBalancerStatusInfo();
if (isRunning) {
- Instant startedAtInstant =
Instant.ofEpochSecond(balancerStatusInfo.getStartedAt());
- LocalDateTime dateTime =
- LocalDateTime.ofInstant(startedAtInstant, ZoneId.systemDefault());
System.out.println("ContainerBalancer is Running.");
+ } else if (response.hasContainerBalancerStatusInfo()) {
+ System.out.println("ContainerBalancer is Not Running.");
+ if (balancerStatusInfo.hasStopReason()) {
+ System.out.printf("Stop reason: %s%n",
balancerStatusInfo.getStopReason());
+ }
+ } else {
+ System.out.println("ContainerBalancer is Not Running.");
+ }
- if (isVerbose()) {
- System.out.printf("Started at: %s %s%n",
- dateTime.toLocalDate().format(DateTimeFormatter.ISO_LOCAL_DATE),
- dateTime.toLocalTime().format(DateTimeFormatter.ISO_LOCAL_TIME));
- Duration balancingDuration = Duration.between(startedAtInstant,
OffsetDateTime.now());
- System.out.printf("Balancing duration: %s%n%n",
getPrettyDuration(balancingDuration));
-
System.out.println(getConfigurationPrettyString(balancerStatusInfo.getConfiguration()));
- List<ContainerBalancerTaskIterationStatusInfoProto>
iterationsStatusInfoList
- = balancerStatusInfo.getIterationsStatusInfoList();
+ if (isVerbose() && response.hasContainerBalancerStatusInfo()) {
+ printVerboseStatusInfo(balancerStatusInfo, isRunning);
+ }
+ }
- System.out.println("Current iteration info:");
- ContainerBalancerTaskIterationStatusInfoProto
currentIterationStatistic = iterationsStatusInfoList.stream()
- .filter(it -> it.getIterationResult().isEmpty())
- .findFirst()
- .orElse(null);
- if (currentIterationStatistic == null) {
- System.out.println("-");
- System.out.println();
- } else {
- System.out.println(
- getPrettyIterationStatusInfo(currentIterationStatistic)
- );
- }
+ private void printVerboseStatusInfo(ContainerBalancerStatusInfoProto
balancerStatusInfo, boolean isRunning) {
+ Instant startedAtInstant =
Instant.ofEpochSecond(balancerStatusInfo.getStartedAt());
+ LocalDateTime startedAtDateTime =
+ LocalDateTime.ofInstant(startedAtInstant, ZoneId.systemDefault());
+ System.out.printf("Started at: %s %s%n",
+
startedAtDateTime.toLocalDate().format(DateTimeFormatter.ISO_LOCAL_DATE),
+
startedAtDateTime.toLocalTime().format(DateTimeFormatter.ISO_LOCAL_TIME));
+ Instant endInstant = balancerStatusInfo.hasStoppedAt()
+ ? Instant.ofEpochSecond(balancerStatusInfo.getStoppedAt())
+ : OffsetDateTime.now().toInstant();
+ if (balancerStatusInfo.hasStoppedAt()) {
+ LocalDateTime stoppedAtDateTime =
+ LocalDateTime.ofInstant(endInstant, ZoneId.systemDefault());
+ System.out.printf("Stopped at: %s %s%n",
+
stoppedAtDateTime.toLocalDate().format(DateTimeFormatter.ISO_LOCAL_DATE),
+
stoppedAtDateTime.toLocalTime().format(DateTimeFormatter.ISO_LOCAL_TIME));
+ }
+ Duration balancingDuration = Duration.between(startedAtInstant,
endInstant);
+ System.out.printf("Balancing duration: %s%n%n",
getPrettyDuration(balancingDuration));
+
System.out.println(getConfigurationPrettyString(balancerStatusInfo.getConfiguration()));
+ List<ContainerBalancerTaskIterationStatusInfoProto>
iterationsStatusInfoList =
+ balancerStatusInfo.getIterationsStatusInfoList();
- if (verboseWithHistory) {
- System.out.println("Iteration history list:");
- System.out.println(
- iterationsStatusInfoList
- .stream()
- .filter(it -> !it.getIterationResult().isEmpty())
- .map(this::getPrettyIterationStatusInfo)
- .collect(Collectors.joining(System.lineSeparator()))
- );
- }
+ if (isRunning) {
+ System.out.println("Current iteration info:");
+ ContainerBalancerTaskIterationStatusInfoProto currentIterationStatistic
= iterationsStatusInfoList.stream()
+ .filter(it -> it.getIterationResult().isEmpty())
+ .findFirst()
+ .orElse(null);
+ if (currentIterationStatistic == null) {
+ System.out.println("-");
+ System.out.println();
+ } else {
+ System.out.println(
+ getPrettyIterationStatusInfo(currentIterationStatistic)
+ );
}
-
} else {
- System.out.println("ContainerBalancer is Not Running.");
+ System.out.println("Last iteration info:");
+ ContainerBalancerTaskIterationStatusInfoProto lastIterationStatistic =
iterationsStatusInfoList.stream()
+ .filter(it -> !it.getIterationResult().isEmpty())
+ .reduce((first, second) -> second)
+ .orElse(null);
+ if (lastIterationStatistic == null) {
+ System.out.println("-");
+ System.out.println();
+ } else {
+ System.out.println(
+ getPrettyIterationStatusInfo(lastIterationStatistic)
+ );
+ }
+ }
+
+ if (verboseWithHistory) {
+ System.out.println("Iteration history list:");
+ System.out.println(
+ iterationsStatusInfoList
+ .stream()
+ .filter(it -> !it.getIterationResult().isEmpty())
Review Comment:
While running with no completed iterations, `Collectors.joining` on the
empty stream prints the header followed by a blank line (visible in the running
`--verbose -H` sample). Every other empty section prints `-` history should
too, for consistency.
--
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]
