You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Gargi-jais11 (via GitHub)" <gi...@apache.org> on 2025/03/25 04:24:37 UTC
[PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Gargi-jais11 opened a new pull request, #8150:
URL: https://github.com/apache/ozone/pull/8150
## What changes were proposed in this pull request?
As VolumeDensity is already covered by report sub command, we will show bytesMoved (bytesBalanced) in status command.
Besides, the meaning of VolumeDensity is clear to end user, but the real value indicating is not that straight forward. Use the estimated pending move size([HDDS-12437](https://issues.apache.org/jira/browse/HDDS-12437)) will help user to clearly understand how much data size need to move. And this bytesMoved is also a straight forward value, indicating the work disk balancer has done.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12546
## How was this patch tested?
Tested manually by running docker cluster locally.
```
bash-5.1$ ozone admin datanode diskbalancer status
Status result:
Datanode BalancedBytes Status Threshold(%) BandwidthInMB Threads SuccessMove FailureMove EstBytesToMove(MB) EstTimeLeft(min)
ozone-datanode-1.ozone_default 8082423808 RUNNING 0.0002 10 5 7 3 419 1
ozone-datanode-2.ozone_default 9525264384 RUNNING 0.0002 10 5 8 2 812 2
Note: Estimated time left is calculated based on the estimated bytes to move and the configured disk bandwidth.
```
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2016004935
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStatusSubcommand.java:
##########
@@ -90,6 +89,7 @@ private String generateStatus(
String.valueOf(proto.getDiskBalancerConf().getParallelThread()));
contentList.add(String.valueOf(proto.getSuccessMoveCount()));
contentList.add(String.valueOf(proto.getFailureMoveCount()));
+ contentList.add(String.valueOf(proto.getBytesMoved() / (1024 * 1024)));
Review Comment:
@Gargi-jais11 , please add unit test to test bytesToMoveMB, bytesMovedMB and calculateEstimatedTimeLeft, make sure when bytesToMove and bytesMoved are less than 1MB, bytesToMoveMB, bytesMovedMB and calculateEstimatedTimeLeft will show reasonable values.
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2015597610
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStatusSubcommand.java:
##########
@@ -90,6 +89,7 @@ private String generateStatus(
String.valueOf(proto.getDiskBalancerConf().getParallelThread()));
contentList.add(String.valueOf(proto.getSuccessMoveCount()));
contentList.add(String.valueOf(proto.getFailureMoveCount()));
+ contentList.add(String.valueOf(proto.getBytesMoved() / (1024 * 1024)));
Review Comment:
in calculateEstimatedTimeLeft round of is already done to handle case where time is less that 1 min. I do it for others as well.
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #8150:
URL: https://github.com/apache/ozone/pull/8150
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2015543934
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStatusSubcommand.java:
##########
@@ -90,6 +89,7 @@ private String generateStatus(
String.valueOf(proto.getDiskBalancerConf().getParallelThread()));
contentList.add(String.valueOf(proto.getSuccessMoveCount()));
contentList.add(String.valueOf(proto.getFailureMoveCount()));
+ contentList.add(String.valueOf(proto.getBytesMoved() / (1024 * 1024)));
Review Comment:
If proto.getBytesMoved() is less than 1MB， please round up it to 1MB， otherwise, there will be case such as SuccessMove 1, and BytesMoved 0.
Do the same round up for bytesToMoveMB.
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2013614904
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##########
@@ -299,7 +298,6 @@ public DiskBalancerReportProto getDiskBalancerReportProto() {
DiskBalancerReportProto.Builder builder =
DiskBalancerReportProto.newBuilder();
return builder.setIsRunning(shouldRun)
- .setBalancedBytes(totalBalancedBytes.get())
Review Comment:
Keep this one.
@Gargi-jais11 , please check whether both getDiskBalancerReportProto() and getDiskBalancerInfo() are needed to report to SCM. It looks like some duplication here. If we can consolidate getDiskBalancerReportProto and getDiskBalancerInfo into one, that will make the code clean and more understandable. If the consolidation is feasible, you can open a new JIRA to do that.
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2024514219
##########
hadoop-hdds/tools/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestDiskBalancerSubCommand.java:
##########
@@ -192,6 +198,51 @@ public void testDiskBalancerStopSubcommand() throws IOException {
stopCmd.setAllHosts(false);
}
+ public static Stream<Arguments> values() {
+ return Stream.of(
+ Arguments.arguments(0L, 0L, 0L, 0L, 0L), // bytesMovedMB = 0, bytesToMoveMB = 0, estimatedTimeLeft = 0
+ Arguments.arguments(512L, 512L, 1L, 1L, 1L), // bytesMoved and bytesToMove < 1MB should be rounded up to 1MB
+ Arguments.arguments(5242880L, 10485760L, 5L, 10L, 1L), // bytesMoved = 5MB, bytesToMove = 10MB, estTimeLeft = 1
+ Arguments.arguments(13774139392L, 3229900800L, 13137L, 3081L, 6L),
+ Arguments.arguments(7482638336L, 939524096L, 7136L, 896L, 6L)
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("values")
+ public void testDiskBalancerStatusCalculations(long bytesMoved, long bytesToMove, long bytesMovedMB,
+ long bytesToMoveMB, long estTimeLeft) throws IOException {
+ ScmClient scmClient = mock(ScmClient.class);
+
+ HddsProtos.DatanodeDiskBalancerInfoProto proto =
+ HddsProtos.DatanodeDiskBalancerInfoProto.newBuilder()
+ .setNode(generateDatanodeDetails())
+ .setCurrentVolumeDensitySum(random.nextDouble())
+ .setRunningStatus(HddsProtos.DiskBalancerRunningStatus.
+ valueOf(random.nextInt(2) + 1))
+ .setBytesMoved(bytesMoved)
+ .setBytesToMove(bytesToMove)
+ .setDiskBalancerConf(
+ HddsProtos.DiskBalancerConfigurationProto.newBuilder()
+ .setDiskBandwidthInMB(10)
+ .setThreshold(10.0)
+ .setParallelThread(5)
+ .build())
+ .build();
+
+ List<HddsProtos.DatanodeDiskBalancerInfoProto> resultList = Collections.singletonList(proto);
+ Mockito.when(scmClient.getDiskBalancerStatus(Mockito.any(), Mockito.any())).thenReturn(resultList);
+
+ DiskBalancerStatusSubcommand statusCmd1 = new DiskBalancerStatusSubcommand();
+ statusCmd1.execute(scmClient);
+
+ String output = outContent.toString(DEFAULT_ENCODING).trim();
+
+ // Check if expected values appear in output
+ assertTrue(output.contains(String.valueOf(bytesMovedMB)));
Review Comment:
Please separate the output to sub strings, and directly compare the matched column with this expected value. The current contains check is not accurate. For example, the last 6L is wrong. But the assert just passed.
Arguments.arguments(7482638336L, 939524096L, 7136L, 896L, 6L)
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2013617116
##########
hadoop-hdds/interface-client/src/main/proto/hdds.proto:
##########
@@ -564,4 +564,5 @@ message DatanodeDiskBalancerInfoProto {
optional uint64 successMoveCount = 5;
optional uint64 failureMoveCount = 6;
optional uint64 bytesToMove = 7;
+ optional uint64 balancedBytes = 8;
Review Comment:
Plase use bytesMoved here.
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "Gargi-jais11 (via GitHub)" <gi...@apache.org>.
Gargi-jais11 commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2024519212
##########
hadoop-hdds/tools/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestDiskBalancerSubCommand.java:
##########
@@ -192,6 +198,51 @@ public void testDiskBalancerStopSubcommand() throws IOException {
stopCmd.setAllHosts(false);
}
+ public static Stream<Arguments> values() {
+ return Stream.of(
+ Arguments.arguments(0L, 0L, 0L, 0L, 0L), // bytesMovedMB = 0, bytesToMoveMB = 0, estimatedTimeLeft = 0
+ Arguments.arguments(512L, 512L, 1L, 1L, 1L), // bytesMoved and bytesToMove < 1MB should be rounded up to 1MB
+ Arguments.arguments(5242880L, 10485760L, 5L, 10L, 1L), // bytesMoved = 5MB, bytesToMove = 10MB, estTimeLeft = 1
+ Arguments.arguments(13774139392L, 3229900800L, 13137L, 3081L, 6L),
+ Arguments.arguments(7482638336L, 939524096L, 7136L, 896L, 6L)
Review Comment:
yaa I think while writing I did mistake.
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2015543934
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStatusSubcommand.java:
##########
@@ -90,6 +89,7 @@ private String generateStatus(
String.valueOf(proto.getDiskBalancerConf().getParallelThread()));
contentList.add(String.valueOf(proto.getSuccessMoveCount()));
contentList.add(String.valueOf(proto.getFailureMoveCount()));
+ contentList.add(String.valueOf(proto.getBytesMoved() / (1024 * 1024)));
Review Comment:
If proto.getBytesMoved() is less than 1MB， please round up it to 1MB， otherwise, there will be case such as SuccessMove 1, and BytesMoved 0.
Do the same round up for bytesToMoveMB here and in calculateEstimatedTimeLeft.
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2024509871
##########
hadoop-hdds/tools/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestDiskBalancerSubCommand.java:
##########
@@ -192,6 +198,51 @@ public void testDiskBalancerStopSubcommand() throws IOException {
stopCmd.setAllHosts(false);
}
+ public static Stream<Arguments> values() {
+ return Stream.of(
+ Arguments.arguments(0L, 0L, 0L, 0L, 0L), // bytesMovedMB = 0, bytesToMoveMB = 0, estimatedTimeLeft = 0
+ Arguments.arguments(512L, 512L, 1L, 1L, 1L), // bytesMoved and bytesToMove < 1MB should be rounded up to 1MB
+ Arguments.arguments(5242880L, 10485760L, 5L, 10L, 1L), // bytesMoved = 5MB, bytesToMove = 10MB, estTimeLeft = 1
+ Arguments.arguments(13774139392L, 3229900800L, 13137L, 3081L, 6L),
+ Arguments.arguments(7482638336L, 939524096L, 7136L, 896L, 6L)
Review Comment:
Here the estTimeLeft are both 6, one must be wrong.
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12546. [DiskBalancer] Display bytesMoved instead of VolumeDensity in status output [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8150:
URL: https://github.com/apache/ozone/pull/8150#discussion_r2013620229
##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStatusSubcommand.java:
##########
@@ -64,7 +64,7 @@ private String generateStatus(
List<String> contentList = new ArrayList<>();
contentList.add("Datanode");
- contentList.add("VolumeDensity");
+ contentList.add("BalancedBytes");
Review Comment:
Let's move the BalancedBytes or BytesMoved before "EstBytesToMove", and show the MB value.
--
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-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org