[GitHub] [ozone] rakeshadr commented on a change in pull request #2689: HDDS-5788. Reduce run time for TestOzoneManagerHA tests

[GitBox <gi...@apache.org>]

rakeshadr commented on a change in pull request #2689:
URL: https://github.com/apache/ozone/pull/2689#discussion_r718182900



##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java
##########
@@ -171,22 +173,29 @@ public void init() throws Exception {
      */
     conf.set(OZONE_BLOCK_DELETING_SERVICE_INTERVAL, "10s");
     conf.set(OZONE_KEY_DELETING_LIMIT_PER_TASK, "2");
-    additionalConfiguration();
 
     clusterBuilder = MiniOzoneCluster.newOMHABuilder(conf)
         .setClusterId(clusterId)
         .setScmId(scmId)
         .setOMServiceId(omServiceId)
         .setOmId(omId)
         .setNumOfOzoneManagers(numOfOMs);
-    additionalClusterSettings();
 
-    cluster = (MiniOzoneOMHAClusterImpl)clusterBuilder.build();
+    cluster = (MiniOzoneOMHAClusterImpl) clusterBuilder.build();
     cluster.waitForClusterToBeReady();
     objectStore = OzoneClientFactory.getRpcClient(omServiceId, conf)
         .getObjectStore();
   }
 
+  /**
+   * Apply additional configuration between tests.
+   */
+  @Before
+  public void setupTest() {

Review comment:
       @JyotinderSingh As the cluster is already created before executing the `setupTest()` function, there is no impact on adding these configurations. I think, we can remove this method safely because this was added to speed up the cluster setup for each test case and no functional impact. Now, we are very much better by creating cluster only once for all the test cases.
   
   IMHO, we can safely remove #additionalConfiguration() and #additionalClusterSettings() methods from everywhere in the test classes.

##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerPrepare.java
##########
@@ -175,7 +184,11 @@ public void testPrepareDownedOM() throws Exception {
 
   @Test
   public void testPrepareWithRestart() throws Exception {
+    // Create fresh cluster for this test to prevent timeout from restarting

Review comment:
       @JyotinderSingh  Do we need a new cluster? I ran 10 times in my laptop without a new cluster(without init() and setup() calls ) and passed consistently. Please double check it. Also, if we need to setup new cluster then please shutdown the previously instantiated cluster by invoking `shutdown();` function before `init()`.
   ```
       // Create fresh cluster for this test to prevent timeout from restarting
       // modified cluster.
       init();
       setup();
   ```




-- 
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