rakeshadr commented on code in PR #10681:
URL: https://github.com/apache/ozone/pull/10681#discussion_r3541839614
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -417,21 +427,59 @@ private long getCurrentIterationDuration() {
/**
* Logs the reason for stop and save configuration and stop the task.
*
- * @param stopReason a string specifying the reason for stop
+ * @param reason a string specifying the reason for stop
*/
- private void tryStopWithSaveConfiguration(String stopReason) {
+ private void tryStopWithSaveConfiguration(String reason) {
synchronized (this) {
try {
- LOG.info("Save Configuration for stopping. Reason: {}", stopReason);
+ recordStopReason(reason);
Review Comment:
This records stop reason before `saveConfiguration()` succeeds. If saving
fails, balancer may continue running but the stop reason has already been set.
Please record the reason only after the stop transition.
```
saveConfiguration(config, false, 0);
recordStopReason(reason);
stop();
```
##########
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:
`recordStopReason(ContainerBalancerStopReason.ERROR, e.getMessage());`
For an exception, don’t bake the exception text with the stopped state. Keep
it separate:
```
So the wire/API stays stable:
Stop reason: ERROR
Stop message: Stopped because of an unexpected error. Details: <exception>
```
##########
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:");
Review Comment:
```
Last iteration info:
Key Value
Iteration number 2
Iteration duration 0s
Iteration result CAN_NOT_BALANCE_ANY_MORE
Size scheduled to move 0 B
Moved data size 0 B
Scheduled to move containers 0
Already moved containers 0
Failed to move containers 0
Failed to move containers by timeout 0
Entered data to nodes
-
Exited data from nodes
-
```
In the example that you had pasted in the PR description, `Iteration 2
appears twice:`. Please check and avoid if its duplicating.
##########
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(
Review Comment:
Currently its ending empty string. This looks like the command truncated or
failed to print data.
`Iteration history list:`
Also, can you rename the heading like,
```
Completed iteration history:
-
```
##########
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());
Review Comment:
Current non-verbose output:
`Stop reason: CAN_NOT_BALANCE_ANY_MORE`
This is parseable but not ideal for humans. Conversely:
`Stop reason: Completed all iterations.`
is human-friendly but not stable.
How about splitting like,
```
Stop reason: COMPLETED_ALL_ITERATIONS
Message: Completed configured number of iterations.
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java:
##########
@@ -357,6 +371,7 @@ public void stop() {
return;
}
LOG.info("Trying to stop ContainerBalancer in this SCM.");
+ task.recordStopReason("STOPPED_ON_SCM_STATE_CHANGE");
Review Comment:
Instead of string, please use enum with a human-readable message.
```
enum ContainerBalancerStopReason {
USER_REQUESTED("Stopped by user request."),
SCM_STATE_CHANGE("Stopped because SCM state changed."),
COMPLETED_ALL_ITERATIONS("Completed all configured iterations."),
CAN_NOT_BALANCE_ANY_MORE("No more eligible container moves were found."),
INITIALIZATION_FAILED("Failed to initialize a container balancer
iteration."),
ERROR("Stopped because of an unexpected error."),
UNKNOWN("Stopped for an unknown reason.");
```
##########
hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto:
##########
@@ -658,6 +658,8 @@ message ContainerBalancerStatusInfoProto {
optional uint64 startedAt = 1;
optional ContainerBalancerConfigurationProto configuration = 2;
repeated ContainerBalancerTaskIterationStatusInfoProto iterationsStatusInfo
= 3;
+ optional uint64 stoppedAt = 4;
+ optional string stopReason = 5;
Review Comment:
Please add human-readable text.
`optional string stopMessage = 6;`
##########
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()) {
Review Comment:
Can we think of adding explicit state(enum ContainerBalancerRunState)
instead of overloading boolean. If this requires more code change, testing etc
then please raise another sub-task(sub-jira) and improve the code.
```
enum ContainerBalancerRunState {
NOT_STARTED = 0;
RUNNING = 1;
STOPPING = 2;
STOPPED = 3;
}
optional ContainerBalancerRunState runState = 6;
```
Then CLI:
```
switch (balancerStatusInfo.getRunState()) {
case RUNNING:
System.out.println("ContainerBalancer is Running.");
break;
case STOPPING:
System.out.println("ContainerBalancer is Stopping.");
break;
case STOPPED:
System.out.println("ContainerBalancer is Not Running.");
break;
default:
System.out.println("ContainerBalancer is Not Running.");
}
```
--
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]
