tanvipenumudy commented on code in PR #6241:
URL: https://github.com/apache/ozone/pull/6241#discussion_r1522558020
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestContainerBalancerOperations.java:
##########
@@ -115,4 +125,47 @@ public void testContainerBalancerCLIOperations() throws
Exception {
}
//TODO: add more acceptance after container balancer is fully completed
+
+ /**
+ * Test if Container Balancer CLI options override the default
configurations.
+ */
+ @Test
+ public void testIfCBCLIOverridesConfigs() throws Exception {
+ boolean running = containerBalancerClient.getContainerBalancerStatus();
+ assertFalse(running);
+
+ //CLI option for threshold is not passed
+ Optional<Double> threshold = Optional.empty();
+
+ //CLI options are passed
+ 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> iterations = Optional.of(10000);
+ Optional<Long> moveTimeout = Optional.of(65L);
+ Optional<Long> moveReplicationTimeout = Optional.of(55L);
+ Optional<Long> balancingInterval = Optional.of(70L);
+ Optional<Boolean> networkTopologyEnable = Optional.of(true);
+ 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);
+
+ ContainerBalancerConfiguration config =
cluster.getStorageContainerManager().getContainerBalancer().getConfig();
+
+ //If CLI option is not passed, it takes the default configuration
+ assertEquals(config.getThreshold(), 10);
+
+ //If CLI option is passed, it overrides the default configuration
+ assertEquals(config.getIterations(), 10000);
Review Comment:
Similar to the previous comment.
```suggestion
assertEquals(10000, config.getIterations());
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestContainerBalancerOperations.java:
##########
@@ -115,4 +125,47 @@ public void testContainerBalancerCLIOperations() throws
Exception {
}
//TODO: add more acceptance after container balancer is fully completed
+
+ /**
+ * Test if Container Balancer CLI options override the default
configurations.
+ */
+ @Test
+ public void testIfCBCLIOverridesConfigs() throws Exception {
+ boolean running = containerBalancerClient.getContainerBalancerStatus();
+ assertFalse(running);
+
+ //CLI option for threshold is not passed
+ Optional<Double> threshold = Optional.empty();
+
+ //CLI options are passed
+ 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> iterations = Optional.of(10000);
+ Optional<Long> moveTimeout = Optional.of(65L);
+ Optional<Long> moveReplicationTimeout = Optional.of(55L);
+ Optional<Long> balancingInterval = Optional.of(70L);
+ Optional<Boolean> networkTopologyEnable = Optional.of(true);
+ 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);
+
+ ContainerBalancerConfiguration config =
cluster.getStorageContainerManager().getContainerBalancer().getConfig();
+
+ //If CLI option is not passed, it takes the default configuration
+ assertEquals(config.getThreshold(), 10);
+
+ //If CLI option is passed, it overrides the default configuration
+ assertEquals(config.getIterations(), 10000);
+ assertEquals(config.getNetworkTopologyEnable(), true);
Review Comment:
Similar to the previous comments, perhaps we can go with
`assertTrue(config.getNetworkTopologyEnable())` instead of `assertEquals(true,
config.getNetworkTopologyEnable())` for better readability.
```suggestion
assertTrue(config.getNetworkTopologyEnable());
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestContainerBalancerOperations.java:
##########
@@ -115,4 +125,47 @@ public void testContainerBalancerCLIOperations() throws
Exception {
}
//TODO: add more acceptance after container balancer is fully completed
+
+ /**
+ * Test if Container Balancer CLI options override the default
configurations.
+ */
+ @Test
+ public void testIfCBCLIOverridesConfigs() throws Exception {
+ boolean running = containerBalancerClient.getContainerBalancerStatus();
+ assertFalse(running);
+
+ //CLI option for threshold is not passed
+ Optional<Double> threshold = Optional.empty();
+
+ //CLI options are passed
+ 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> iterations = Optional.of(10000);
+ Optional<Long> moveTimeout = Optional.of(65L);
+ Optional<Long> moveReplicationTimeout = Optional.of(55L);
+ Optional<Long> balancingInterval = Optional.of(70L);
+ Optional<Boolean> networkTopologyEnable = Optional.of(true);
+ 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);
+
+ ContainerBalancerConfiguration config =
cluster.getStorageContainerManager().getContainerBalancer().getConfig();
+
+ //If CLI option is not passed, it takes the default configuration
+ assertEquals(config.getThreshold(), 10);
Review Comment:
Although it is functionally the same in this case since the code works, it's
generally preferred to follow the convention of providing the `expected` value
first followed by the `actual` value when using `Assertions.assertEquals`
(which should also serve helpful while debugging failed tests).
```suggestion
assertEquals(10, config.getThreshold());
```
--
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]
