siddhantsangwan commented on code in PR #9047:
URL: https://github.com/apache/ozone/pull/9047#discussion_r2368423064
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestContainerBalancerOperations.java:
##########
@@ -178,4 +180,44 @@ public void testIfCBCLIOverridesConfigs() throws Exception
{
running = containerBalancerClient.getContainerBalancerStatus();
assertFalse(running);
}
+
+ /**
+ * Tests that stopBalancer is idempotent - once the balancer is in STOPPED
state,
+ * invoking stop again should be a no-op and return successfully with exit
code 0.
+ */
+ @Test
+ public void testStopBalancerIdempotent() throws IOException {
+ boolean running = containerBalancerClient.getContainerBalancerStatus();
+ assertFalse(running);
+
+ Optional<Double> threshold = Optional.of(0.1);
+ Optional<Integer> iterations = Optional.of(10000);
+ Optional<Integer> maxDatanodesPercentageToInvolvePerIteration =
+ Optional.of(100);
+ Optional<Long> maxSizeToMovePerIterationInGB = Optional.of(1L);
+ Optional<Long> maxSizeEnteringTargetInGB = Optional.of(6L);
+ Optional<Long> maxSizeLeavingSourceInGB = Optional.of(6L);
+ Optional<Integer> balancingInterval = Optional.of(70);
+ Optional<Integer> moveTimeout = Optional.of(65);
+ Optional<Integer> moveReplicationTimeout = Optional.of(50);
+ Optional<Boolean> networkTopologyEnable = Optional.of(false);
+ Optional<String> includeNodes = Optional.of("");
+ Optional<String> excludeNodes = Optional.of("");
+ containerBalancerClient.startContainerBalancer(threshold, iterations,
+ maxDatanodesPercentageToInvolvePerIteration,
+ maxSizeToMovePerIterationInGB, maxSizeEnteringTargetInGB,
+ maxSizeLeavingSourceInGB, balancingInterval, moveTimeout,
+ moveReplicationTimeout, networkTopologyEnable, includeNodes,
+ excludeNodes);
+ running = containerBalancerClient.getContainerBalancerStatus();
+ assertTrue(running);
+
+ containerBalancerClient.stopContainerBalancer();
+ running = containerBalancerClient.getContainerBalancerStatus();
+ assertFalse(running);
+
+ // Calling stop balancer multiple times should not throw an exception
+ assertDoesNotThrow(() -> containerBalancerClient.stopContainerBalancer());
+ assertDoesNotThrow(() -> containerBalancerClient.stopContainerBalancer());
Review Comment:
Instead of calling stop twice here, let's call it once here and once after
line 191 when balancer hasn't been started at all.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java:
##########
@@ -142,9 +143,10 @@ public void testStartBalancerStop() throws Exception {
stopBalancer();
assertSame(ContainerBalancerTask.Status.STOPPED,
containerBalancer.getBalancerStatus());
- assertThrows(Exception.class,
- () -> containerBalancer.stopBalancer(),
- "Exception should be thrown when stop again");
+ // If the balancer is already stopped, the stop command should do nothing
+ // and return successfully with exit code 0 as stopBalancer is idempotent
+ assertDoesNotThrow(() -> containerBalancer.stopBalancer());
Review Comment:
Let's move one of these stop calls to the beginning of this test and add a
comment mentioning that stop should not throw because it's idempotent.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancer.java:
##########
@@ -142,9 +143,10 @@ public void testStartBalancerStop() throws Exception {
stopBalancer();
assertSame(ContainerBalancerTask.Status.STOPPED,
containerBalancer.getBalancerStatus());
- assertThrows(Exception.class,
- () -> containerBalancer.stopBalancer(),
- "Exception should be thrown when stop again");
+ // If the balancer is already stopped, the stop command should do nothing
+ // and return successfully with exit code 0 as stopBalancer is idempotent
Review Comment:
Don't really need to mention exit code 0 in this context.
--
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]
