Copilot commented on code in PR #9465:
URL: https://github.com/apache/ozone/pull/9465#discussion_r2618396803
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerService.java:
##########
@@ -256,6 +258,27 @@ public void testCalculateBytesToMove(int volumeCount, int
deltaUsagePercent,
// data precision loss due to double data involved in calculation
assertTrue(Math.abs(expectedBytesToMove -
svc.calculateBytesToMove(immutableVolumes)) <= 1);
+
+ // Test getDiskBalancerInfo() behavior with inProgressContainers check
+ // Set operational state to RUNNING
+ DiskBalancerInfo info = new DiskBalancerInfo(
+ DiskBalancerRunningStatus.RUNNING, 10.0d, 100L, 1,
+ false, DiskBalancerVersion.DEFAULT_VERSION);
+ svc.refresh(info);
+
+ // When inProgressContainers is empty, bytesToMove should be 0
+ assertTrue(svc.getInProgressContainers().isEmpty());
+ DiskBalancerInfo diskBalancerInfo = svc.getDiskBalancerInfo();
+ assertEquals(0, diskBalancerInfo.getBytesToMove(),
+ "bytesToMove should be 0 when no containers are in progress");
+
+ // When inProgressContainers is not empty, bytesToMove should match
expected value
+ svc.getInProgressContainers().add(ContainerID.valueOf(1L));
+ assertFalse(svc.getInProgressContainers().isEmpty());
+ diskBalancerInfo = svc.getDiskBalancerInfo();
+ // data precision loss due to double data involved in calculation
+ assertTrue(Math.abs(expectedBytesToMove -
diskBalancerInfo.getBytesToMove()) <= 1,
+ "bytesToMove should match expected value when containers are in
progress");
Review Comment:
The test logic has an issue with parameterized test cases. The new test
added at lines 275-281 verifies that when containers are in progress,
bytesToMove should match the expected value. However, many of the parameterized
test cases (from the values() method) have expectedBytesToMovePercent of 0,
which means expectedBytesToMove will also be 0. This causes the assertion at
line 280 to pass trivially for those cases (like volumeCount=0, 1, or when
deltaUsagePercent=0), which doesn't properly validate the fix. Consider either
filtering test cases where expectedBytesToMove is 0, or making this a separate
test with specific scenarios where expectedBytesToMove is non-zero.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerConfiguration.java:
##########
@@ -200,9 +200,9 @@ public double getThresholdAsRatio() {
* @param threshold a percentage value in the range 0 to 100
Review Comment:
The javadoc comment for the setThreshold method still states "a percentage
value in the range 0 to 100" but the implementation now excludes both 0 and
100. The javadoc should be updated to clarify that both bounds are exclusive,
for example: "a percentage value in the range (0, 100) exclusive".
```suggestion
* @param threshold a percentage value in the range (0, 100) exclusive
```
--
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]
