You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2021/01/08 11:29:18 UTC

[GitHub] [hbase] wchevreuil commented on a change in pull request #2855: HBASE-25475: Improve UT added as part of HBASE-25445 in TestSplitWALManager

wchevreuil commented on a change in pull request #2855:
URL: https://github.com/apache/hbase/pull/2855#discussion_r553891216



##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java
##########
@@ -131,14 +131,14 @@ public void testWALArchiveWithDifferentWalAndRootFS() throws Exception{
     }
     executor.submitProcedure(splitProcedure);
     LOG.info("Submitted SplitProcedure.");
-    test_util_2.waitFor(30000, () -> executor.getProcedures().stream()
-      .filter(p -> p instanceof TransitRegionStateProcedure)
-      .map(p -> (TransitRegionStateProcedure) p)
-      .anyMatch(p -> TABLE_NAME.equals(p.getTableName())));
-    test_util_2.getMiniHBaseCluster().killRegionServer(
-      test_util_2.getMiniHBaseCluster().getRegionServer(0).getServerName());
-    test_util_2.getMiniHBaseCluster().startRegionServer();
-    test_util_2.waitUntilNoRegionsInTransition();
+    testUtil2.waitFor(30000, () -> executor.getProcedures().stream()
+      .filter(p -> p instanceof SplitTableRegionProcedure)
+      .map(p -> (SplitTableRegionProcedure) p)
+      .anyMatch(p -> TABLE_NAME.equals(p.getTableName())) && splitProcedure.isSuccess());
+    testUtil2.getMiniHBaseCluster().killRegionServer(
+      testUtil2.getMiniHBaseCluster().getRegionServer(0).getServerName());
+    testUtil2.getMiniHBaseCluster().startRegionServer();
+    testUtil2.waitUntilNoRegionsInTransition();

Review comment:
       Test looks fine, functionally wise, but could we simplify it and make it more clear by directly testing the log splitting? Sorry, don't want to be picky, it just looks we are doing more than needed here, maybe it would be simpler to just to do same thing as `testSplitLogs()`, after making sure wal is under a different file system. You could move current `testSplitLogs()` to a separate method that would be called by both this test and `testSplitLogs`. 
   
   WDYT, @dasanjan1296 ? 




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org