ivanzlenko commented on code in PR #7139:
URL: https://github.com/apache/ozone/pull/7139#discussion_r1795249706
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java:
##########
@@ -40,6 +40,10 @@ public final class ContainerBalancerMetrics {
" in the latest iteration.")
private MutableCounterLong dataSizeMovedGBInLatestIteration;
+ @Metric(about = "Amount of bytes that Container Balancer moved" +
Review Comment:
We do not need line break here
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java:
##########
@@ -154,6 +158,24 @@ public void resetDataSizeMovedGBInLatestIteration() {
-getDataSizeMovedGBInLatestIteration());
}
+ /**
+ * Gets the amount of data moved by Container Balancer in the latest
+ * iteration.
+ * @return size in bytes
+ */
+ public long getDataSizeMovedInLatestIteration() {
+ return dataSizeMovedBytesInLatestIteration.value();
+ }
+
+ public void incrementDataSizeMovedInLatestIteration(long valueToAdd) {
Review Comment:
Can you name variable more clear? What are we adding here?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -310,8 +313,9 @@ private void saveIterationStatistic(Integer
iterationNumber, IterationResult iR)
ContainerBalancerTaskIterationStatusInfo iterationStatistic = new
ContainerBalancerTaskIterationStatusInfo(
iterationNumber,
iR.name(),
- getSizeScheduledForMoveInLatestIteration() / OzoneConsts.GB,
- metrics.getDataSizeMovedGBInLatestIteration(),
+ now().toEpochSecond() - currentIterationStarted.toEpochSecond(),
Review Comment:
1. I would've move it into separate variable
2. Align to other parameters
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -720,6 +730,8 @@ private void checkIterationMoveResults() {
metrics.getNumContainerMovesTimeoutInLatestIteration());
metrics.incrementDataSizeMovedGBInLatestIteration(
sizeActuallyMovedInLatestIteration / OzoneConsts.GB);
+ metrics.incrementDataSizeMovedInLatestIteration(
Review Comment:
No need in line break
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/util/DurationUtil.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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;
+
+/**
+ * Pretty duration string representation.
+ */
+public final class DurationUtil {
+
+ private DurationUtil() {
+ }
+
+ public static String getPrettyDuration(long durationSeconds) {
Review Comment:
Please add javaDoc
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java:
##########
@@ -159,20 +169,23 @@ private String
getPrettyIterationStatusInfo(ContainerBalancerTaskIterationStatus
"%-50s %s%n" +
"%-50s %s%n" +
"%-50s %s%n" +
+ "%-50s %s%n" +
"%-50s %n%s" +
"%-50s %n%s",
"Key", "Value",
- "Iteration number", iterationNumber,
+ "Iteration number", iterationNumber == 0 ? "-" : iterationNumber,
Review Comment:
For what reason are we inserting string constants using String.format? It
doesn't feel right quite to be honest. Can we refactor this?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java:
##########
@@ -154,6 +158,24 @@ public void resetDataSizeMovedGBInLatestIteration() {
-getDataSizeMovedGBInLatestIteration());
}
+ /**
+ * Gets the amount of data moved by Container Balancer in the latest
+ * iteration.
+ * @return size in bytes
+ */
+ public long getDataSizeMovedInLatestIteration() {
+ return dataSizeMovedBytesInLatestIteration.value();
+ }
+
+ public void incrementDataSizeMovedInLatestIteration(long valueToAdd) {
+ this.dataSizeMovedBytesInLatestIteration.incr(valueToAdd);
+ }
+
+ public void resetDataSizeMovedInLatestIteration() {
+ dataSizeMovedBytesInLatestIteration.incr(
Review Comment:
Why line break here?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java:
##########
@@ -154,6 +158,24 @@ public void resetDataSizeMovedGBInLatestIteration() {
-getDataSizeMovedGBInLatestIteration());
}
+ /**
+ * Gets the amount of data moved by Container Balancer in the latest
Review Comment:
Line break
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -335,25 +339,23 @@ private void saveIterationStatistic(Integer
iterationNumber, IterationResult iR)
.collect(
Collectors.toMap(
entry -> entry.getKey().getUuid(),
- entry -> entry.getValue() / OzoneConsts.GB
+ entry -> entry.getValue()
)
)
);
iterationsStatistic.add(iterationStatistic);
}
public List<ContainerBalancerTaskIterationStatusInfo>
getCurrentIterationsStatistic() {
-
- int lastIterationNumber = iterationsStatistic.stream()
- .mapToInt(ContainerBalancerTaskIterationStatusInfo::getIterationNumber)
- .max()
- .orElse(0);
-
+ int lastIterationNumber = iterationsStatistic.isEmpty() ? 0 :
Review Comment:
It is either should be on one line or please use following format:
expression
? if true
: or else
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerStatusInfo.java:
##########
@@ -51,4 +53,18 @@ public HddsProtos.ContainerBalancerConfigurationProto
getConfiguration() {
public List<ContainerBalancerTaskIterationStatusInfo>
getIterationsStatusInfo() {
return iterationsStatusInfo;
}
+
+ public StorageContainerLocationProtocolProtos.ContainerBalancerStatusInfo
toProto() {
Review Comment:
Please add javaDoc
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -324,7 +328,7 @@ private void saveIterationStatistic(Integer
iterationNumber, IterationResult iR)
.collect(
Collectors.toMap(
entry -> entry.getKey().getUuid(),
- entry -> entry.getValue() / OzoneConsts.GB
+ Map.Entry::getValue
Review Comment:
Missaligned
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerStatusInfo.java:
##########
@@ -51,4 +53,18 @@ public HddsProtos.ContainerBalancerConfigurationProto
getConfiguration() {
public List<ContainerBalancerTaskIterationStatusInfo>
getIterationsStatusInfo() {
return iterationsStatusInfo;
}
+
+ public StorageContainerLocationProtocolProtos.ContainerBalancerStatusInfo
toProto() {
+ return StorageContainerLocationProtocolProtos.ContainerBalancerStatusInfo
+ .newBuilder()
+ .setStartedAt(getStartedAt().toEpochSecond())
+ .setConfiguration(getConfiguration())
+ .addAllIterationsStatusInfo(
+ getIterationsStatusInfo()
+ .stream()
+ .map(ContainerBalancerTaskIterationStatusInfo::toProto)
+ .collect(Collectors.toList())
+ ).build();
+ }
+
Review Comment:
Unnecessary line break
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerMetrics.java:
##########
@@ -154,6 +158,24 @@ public void resetDataSizeMovedGBInLatestIteration() {
-getDataSizeMovedGBInLatestIteration());
}
+ /**
+ * Gets the amount of data moved by Container Balancer in the latest
+ * iteration.
+ * @return size in bytes
+ */
+ public long getDataSizeMovedInLatestIteration() {
+ return dataSizeMovedBytesInLatestIteration.value();
+ }
+
+ public void incrementDataSizeMovedInLatestIteration(long valueToAdd) {
+ this.dataSizeMovedBytesInLatestIteration.incr(valueToAdd);
+ }
+
+ public void resetDataSizeMovedInLatestIteration() {
Review Comment:
Please add javaDoc
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTaskIterationStatusInfo.java:
##########
@@ -18,46 +18,54 @@
Review Comment:
Can we have javaDoc for public methods please?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -335,25 +339,23 @@ private void saveIterationStatistic(Integer
iterationNumber, IterationResult iR)
.collect(
Collectors.toMap(
entry -> entry.getKey().getUuid(),
- entry -> entry.getValue() / OzoneConsts.GB
+ entry -> entry.getValue()
Review Comment:
You can use method reference here
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/util/DurationUtil.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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;
+
+/**
+ * Pretty duration string representation.
+ */
+public final class DurationUtil {
+
+ private DurationUtil() {
+ }
+
+ public static String getPrettyDuration(long durationSeconds) {
+ String prettyDuration;
Review Comment:
Please consider using Duration and String.format to make code clean.
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/ContainerBalancerStatusSubcommand.java:
##########
@@ -78,7 +85,9 @@ public void execute(ScmClient scmClient) throws IOException {
if (verboseWithHistory) {
System.out.println("Iteration history list:");
System.out.println(
-
iterationsStatusInfoList.stream().map(this::getPrettyIterationStatusInfo)
+ iterationsStatusInfoList.subList(0,
iterationsStatusInfoList.size() - 1)
Review Comment:
What exactly are you trying to achieve here? Sublist call feels like
absolutely redundant.
--
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
