siddhantsangwan commented on code in PR #7139:
URL: https://github.com/apache/ozone/pull/7139#discussion_r1818544774
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java:
##########
@@ -218,14 +243,19 @@ public long
getNumContainerMovesTimeoutInLatestIteration() {
return numContainerMovesTimeoutInLatestIteration.value();
}
+ /**
+ * Increases the number of containers that transfer completed with a timeout.
+ */
Review Comment:
Time out means that the container move did not complete. Same for the java
doc below.
##########
hadoop-hdds/tools/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestContainerBalancerSubCommand.java:
##########
@@ -206,27 +178,298 @@ public void
testContainerBalancerStatusInfoSubcommandRunning()
.setStartedAt(OffsetDateTime.now().toEpochSecond())
.setConfiguration(config.toProtobufBuilder().setShouldRun(true))
.addAllIterationsStatusInfo(
- Arrays.asList(iteration0StatusInfo, iteration1StatusInfo,
iteration2StatusInfo)
+ Arrays.asList(iteration1StatusInfo, iteration2StatusInfo,
iteration3StatusInfo)
)
)
.build();
+ return statusInfoResponseProto;
+ }
+
+ private static ContainerBalancerConfiguration
getContainerBalancerConfiguration() {
+ ContainerBalancerConfiguration config = new
ContainerBalancerConfiguration();
+ config.setThreshold(10);
+ config.setMaxDatanodesPercentageToInvolvePerIteration(20);
+ config.setMaxSizeToMovePerIteration(53687091200L);
+ config.setMaxSizeEnteringTarget(27917287424L);
+ config.setMaxSizeLeavingSource(27917287424L);
+ config.setIterations(3);
+ config.setExcludeNodes("");
+ config.setMoveTimeout(3900000);
+ config.setMoveReplicationTimeout(3000000);
+ config.setBalancingInterval(0);
+ config.setIncludeNodes("");
+ config.setExcludeNodes("");
+ config.setNetworkTopologyEnable(false);
+ config.setTriggerDuEnable(false);
+ return config;
+ }
+
+ @BeforeEach
+ public void setup() throws UnsupportedEncodingException {
+ stopCmd = new ContainerBalancerStopSubcommand();
+ startCmd = new ContainerBalancerStartSubcommand();
+ statusCmd = new ContainerBalancerStatusSubcommand();
+ System.setOut(new PrintStream(outContent, false, DEFAULT_ENCODING));
+ System.setErr(new PrintStream(errContent, false, DEFAULT_ENCODING));
+ }
+
+ @AfterEach
+ public void tearDown() {
+ System.setOut(originalOut);
+ System.setErr(originalErr);
+ }
+
+ @Test
+ void testContainerBalancerStatusInfoSubcommandRunningWithoutFlags()
+ throws IOException {
+ ScmClient scmClient = mock(ScmClient.class);
+
+ ContainerBalancerConfiguration config =
+ getContainerBalancerConfiguration();
+
+ ContainerBalancerStatusInfoResponseProto
+ statusInfoResponseProto =
getContainerBalancerStatusInfoResponseProto(config);
//test status is running
when(scmClient.getContainerBalancerStatusInfo()).thenReturn(statusInfoResponseProto);
-
statusCmd.execute(scmClient);
Pattern p = Pattern.compile(
"^ContainerBalancer\\sis\\sRunning.");
- Matcher m = p.matcher(outContent.toString(DEFAULT_ENCODING));
+ String output = outContent.toString(DEFAULT_ENCODING);
+ Matcher m = p.matcher(output);
assertTrue(m.find());
+
+ String balancerConfigOutput =
+ "Container Balancer Configuration values:\n" +
+ "Key Value\n" +
+ "Threshold 10.0\n" +
+ "Max Datanodes to Involve per Iteration(percent) 20\n" +
+ "Max Size to Move per Iteration 0GB\n" +
+ "Max Size Entering Target per Iteration 26GB\n" +
+ "Max Size Leaving Source per Iteration 26GB\n" +
+ "Number of Iterations 3\n" +
+ "Time Limit for Single Container's Movement 65min\n" +
+ "Time Limit for Single Container's Replication 50min\n" +
+ "Interval between each Iteration 0min\n" +
+ "Whether to Enable Network Topology false\n" +
+ "Whether to Trigger Refresh Datanode Usage Info false\n" +
+ "Container IDs to Exclude from Balancing None\n" +
+ "Datanodes Specified to be Balanced None\n" +
+ "Datanodes Excluded from Balancing None";
+ assertFalse(output.contains(balancerConfigOutput));
+
+ String currentIterationOutput =
+ "Current iteration info:\n" +
+ "Key Value\n" +
+ "Iteration number 3\n" +
+ "Iteration duration 1h 6m 40s\n" +
+ "Iteration result IN_PROGRESS\n" +
+ "Size scheduled to move 48 GB\n" +
+ "Moved data size 48 GB\n" +
+ "Scheduled to move containers 11\n" +
+ "Already moved containers 11\n" +
+ "Failed to move containers 0\n" +
+ "Failed to move containers by timeout 0\n" +
+ "Entered data to nodes \n" +
+ "80f6bc27-e6f3-493e-b1f4-25f810ad960d <- 20 GB\n" +
+ "701ca98e-aa1a-4b36-b817-e28ed634bba6 <- 28 GB\n" +
+ "Exited data from nodes \n" +
+ "b8b9c511-c30f-4933-8938-2f272e307070 -> 30 GB\n" +
+ "7bd99815-47e7-4015-bc61-ca6ef6dfd130 -> 18 GB";
+ assertFalse(output.contains(currentIterationOutput));
+
+ assertFalse(output.contains("Iteration history list:"));
}
@Test
- public void
testContainerBalancerStatusInfoSubcommandRunningOnStoppedBalancer()
+ void testContainerBalancerStatusInfoSubcommandVerboseHistory()
+ throws IOException {
+ ScmClient scmClient = mock(ScmClient.class);
+
+ ContainerBalancerConfiguration config =
+ getContainerBalancerConfiguration();
+
+ ContainerBalancerStatusInfoResponseProto
+ statusInfoResponseProto =
getContainerBalancerStatusInfoResponseProto(config);
+ //test status is running
+
when(scmClient.getContainerBalancerStatusInfo()).thenReturn(statusInfoResponseProto);
+ CommandLine c = new CommandLine(statusCmd);
+ c.parseArgs("--verbose", "--history");
+ statusCmd.execute(scmClient);
+ String output = outContent.toString(DEFAULT_ENCODING);
+ Pattern p = Pattern.compile(
+ "^ContainerBalancer\\sis\\sRunning.$", Pattern.MULTILINE);
+ Matcher m = p.matcher(output);
+ assertTrue(m.find());
+
+ p = Pattern.compile(
+ "^Started at: (\\d{4}-\\d{2}-\\d{2} \\d{2}:\\d{2}:\\d{2})$",
Pattern.MULTILINE);
+ m = p.matcher(output);
+ assertTrue(m.find());
+
+ p = Pattern.compile(
+ "^Balancing duration: \\d{1}s$", Pattern.MULTILINE);
+ m = p.matcher(output);
+ assertTrue(m.find());
+
+ String balancerConfigOutput =
+ "Container Balancer Configuration values:\n" +
+ "Key Value\n" +
+ "Threshold 10.0\n" +
+ "Max Datanodes to Involve per Iteration(percent) 20\n" +
+ "Max Size to Move per Iteration 0GB\n" +
+ "Max Size Entering Target per Iteration 26GB\n" +
+ "Max Size Leaving Source per Iteration 26GB\n" +
+ "Number of Iterations 3\n" +
+ "Time Limit for Single Container's Movement 65min\n" +
+ "Time Limit for Single Container's Replication 50min\n" +
+ "Interval between each Iteration 0min\n" +
+ "Whether to Enable Network Topology false\n" +
+ "Whether to Trigger Refresh Datanode Usage Info false\n" +
+ "Container IDs to Exclude from Balancing None\n" +
+ "Datanodes Specified to be Balanced None\n" +
+ "Datanodes Excluded from Balancing None";
+ assertTrue(output.contains(balancerConfigOutput));
+
+ String currentIterationOutput =
+ "Current iteration info:\n" +
+ "Key Value\n" +
+ "Iteration number 3\n" +
+ "Iteration duration 1h 6m 40s\n" +
+ "Iteration result IN_PROGRESS\n" +
+ "Size scheduled to move 48 GB\n" +
+ "Moved data size 48 GB\n" +
+ "Scheduled to move containers 11\n" +
+ "Already moved containers 11\n" +
+ "Failed to move containers 0\n" +
+ "Failed to move containers by timeout 0\n" +
+ "Entered data to nodes \n" +
+ "80f6bc27-e6f3-493e-b1f4-25f810ad960d <- 20 GB\n" +
+ "701ca98e-aa1a-4b36-b817-e28ed634bba6 <- 28 GB\n" +
+ "Exited data from nodes \n" +
+ "b8b9c511-c30f-4933-8938-2f272e307070 -> 30 GB\n" +
+ "7bd99815-47e7-4015-bc61-ca6ef6dfd130 -> 18 GB";
+ assertTrue(output.contains(currentIterationOutput));
+
+ assertTrue(output.contains("Iteration history list:"));
+ String firstHistoryIterationOutput =
+ "Key Value\n" +
+ "Iteration number 3\n" +
+ "Iteration duration 1h 6m 40s\n" +
+ "Iteration result IN_PROGRESS\n" +
+ "Size scheduled to move 48 GB\n" +
+ "Moved data size 48 GB\n" +
+ "Scheduled to move containers 11\n" +
+ "Already moved containers 11\n" +
+ "Failed to move containers 0\n" +
+ "Failed to move containers by timeout 0\n" +
+ "Entered data to nodes \n" +
+ "80f6bc27-e6f3-493e-b1f4-25f810ad960d <- 20 GB\n" +
+ "701ca98e-aa1a-4b36-b817-e28ed634bba6 <- 28 GB\n" +
+ "Exited data from nodes \n" +
+ "b8b9c511-c30f-4933-8938-2f272e307070 -> 30 GB\n" +
+ "7bd99815-47e7-4015-bc61-ca6ef6dfd130 -> 18 GB";
+ assertTrue(output.contains(firstHistoryIterationOutput));
Review Comment:
Are we asserting the same thing twice? It looks like
`currentIterationOutput` and `firstHistoryIterationOutput` have the same
content.
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java:
##########
@@ -60,12 +64,15 @@ public void execute(ScmClient scmClient) throws IOException
{
boolean isRunning = response.getIsRunning();
ContainerBalancerStatusInfo balancerStatusInfo =
response.getContainerBalancerStatusInfo();
if (isRunning) {
+ Instant startedAtInstant =
Instant.ofEpochSecond(balancerStatusInfo.getStartedAt());
LocalDateTime dateTime =
-
LocalDateTime.ofInstant(Instant.ofEpochSecond(balancerStatusInfo.getStartedAt()),
ZoneId.systemDefault());
+ LocalDateTime.ofInstant(startedAtInstant, 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.printf("Started at: %s %s%n", dateTime.toLocalDate(),
dateTime.toLocalTime());
+ Duration balancingDuration = Duration.between(startedAtInstant,
OffsetDateTime.now());
+ System.out.printf("Balancing duration: %s%n%n",
getPrettyDuration(balancingDuration));
Review Comment:
Why not use the `toString` method of `Duration` instead of writing our own?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/IterationInfo.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.container.balancer;
+
+/**
+ * Information about the process of moving data.
+ */
Review Comment:
Looks like you forgot to change this java doc, it's currently not relevant.
##########
hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto:
##########
@@ -628,19 +628,20 @@ message ContainerBalancerStatusInfo {
message ContainerBalancerTaskIterationStatusInfo {
Review Comment:
Note that all other messages in this file have the suffix "Proto". This
really improves code readability because the reader can distinguish between the
java class `ContainerBalancerTaskIterationStatusInfo` and the java class that's
generated by protoc for the proto message.
##########
hadoop-hdds/tools/src/test/java/org/apache/hadoop/hdds/util/DurationUtilTest.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.util;
+
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.time.Duration;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
+
+class DurationUtilTest {
Review Comment:
```suggestion
class TestDurationUtil {
```
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
