HBASE-19839 Fixed flakey tests TestMergeTableRegionsProcedure#testRollbackAndDoubleExecution and TestSplitTableRegionProcedure#testRollbackAndDoubleExecution

* Added a comment in MergeTableRegionsProcedure and SplitTableRegionProcedure explaining specific rollbacks has side effect that AssignProcedure/s are submitted asynchronously and those procedures may continue to execute even after rollback() is done.
* Updated comments in tests with correct rollback state to abort
* Added overloaded method MasterProcedureTestingUtility#testRollbackAndDoubleExecution which takes additional argument for waiting on all procedures to finish before asserting conditions
* Updated TestMergeTableRegionsProcedure#testRollbackAndDoubleExecution and TestSplitTableRegionProcedure#testRollbackAndDoubleExecution to use newly added method

Signed-off-by: Michael Stack <stack@apache.org>

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/MergeTableRegionsProcedure.java

@@ -263,6 +263,12 @@ public class MergeTableRegionsProcedure
     return Flow.HAS_MORE_STATE;
   }
 
+  /**
+   * To rollback {@link MergeTableRegionsProcedure}, two AssignProcedures are asynchronously
+   * submitted for each region to be merged (rollback doesn't wait on the completion of the
+   * AssignProcedures) . This can be improved by changing rollback() to support sub-procedures.
+   * See HBASE-19851 for details.
+   */
   @Override
   protected void rollbackState(
       final MasterProcedureEnv env,

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java

@@ -263,6 +263,12 @@ public class SplitTableRegionProcedure
     return Flow.HAS_MORE_STATE;
   }
 
+  /**
+   * To rollback {@link SplitTableRegionProcedure}, an AssignProcedure is asynchronously
+   * submitted for parent region to be split (rollback doesn't wait on the completion of the
+   * AssignProcedure) . This can be improved by changing rollback() to support sub-procedures.
+   * See HBASE-19851 for details.
+   */
   @Override
   protected void rollbackState(final MasterProcedureEnv env, final SplitTableRegionState state)
       throws IOException, InterruptedException {

hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMergeTableRegionsProcedure.java

@@ -266,11 +266,12 @@ public class TestMergeTableRegionsProcedure {
     long procId = procExec.submitProcedure(
       new MergeTableRegionsProcedure(procExec.getEnvironment(), regionsToMerge, true));
 
-    // Failing before MERGE_TABLE_REGIONS_UPDATE_META we should trigger the rollback
+    // Failing before MERGE_TABLE_REGIONS_CREATE_MERGED_REGION we should trigger the rollback
-    // NOTE: the 5 (number before MERGE_TABLE_REGIONS_UPDATE_META step) is
+    // NOTE: the 5 (number before MERGE_TABLE_REGIONS_CREATE_MERGED_REGION step) is
     // hardcoded, so you have to look at this test at least once when you add a new step.
     int numberOfSteps = 5;
-    MasterProcedureTestingUtility.testRollbackAndDoubleExecution(procExec, procId, numberOfSteps);
+    MasterProcedureTestingUtility.testRollbackAndDoubleExecution(procExec, procId, numberOfSteps,
+        true);
   }
 
   @Test

hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestSplitTableRegionProcedure.java

@@ -366,12 +366,13 @@ public class TestSplitTableRegionProcedure {
     long procId = procExec.submitProcedure(
       new SplitTableRegionProcedure(procExec.getEnvironment(), regions[0], splitKey));
 
-    // Failing before SPLIT_TABLE_REGION_UPDATE_META we should trigger the
+    // Failing before SPLIT_TABLE_REGION_CREATE_DAUGHTER_REGIONS we should trigger the
     // rollback
-    // NOTE: the 3 (number before SPLIT_TABLE_REGION_UPDATE_META step) is
+    // NOTE: the 3 (number before SPLIT_TABLE_REGION_CREATE_DAUGHTER_REGIONS step) is
     // hardcoded, so you have to look at this test at least once when you add a new step.
     int numberOfSteps = 3;
-    MasterProcedureTestingUtility.testRollbackAndDoubleExecution(procExec, procId, numberOfSteps);
+    MasterProcedureTestingUtility.testRollbackAndDoubleExecution(procExec, procId, numberOfSteps,
+        true);
     // check that we have only 1 region
     assertEquals(1, UTIL.getHBaseAdmin().getTableRegions(tableName).size());
     List<HRegion> daughters = UTIL.getMiniHBaseCluster().getRegions(tableName);

hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java

@@ -427,6 +427,12 @@ public class MasterProcedureTestingUtility {
   public static void testRollbackAndDoubleExecution(
       final ProcedureExecutor<MasterProcedureEnv> procExec, final long procId,
       final int lastStep) throws Exception {
+    testRollbackAndDoubleExecution(procExec, procId, lastStep, false);
+  }
+
+  public static void testRollbackAndDoubleExecution(
+      final ProcedureExecutor<MasterProcedureEnv> procExec, final long procId,
+      final int lastStep, boolean waitForAsyncProcs) throws Exception {
     // Execute up to last step
     testRecoveryAndDoubleExecution(procExec, procId, lastStep, false);
 
@@ -448,6 +454,19 @@ public class MasterProcedureTestingUtility {
       assertTrue(procExec.unregisterListener(abortListener));
     }
 
+    if (waitForAsyncProcs) {
+      // Sometimes there are other procedures still executing (including asynchronously spawned by
+      // procId) and due to KillAndToggleBeforeStoreUpdate flag ProcedureExecutor is stopped before
+      // store update. Let all pending procedures finish normally.
+      if (!procExec.isRunning()) {
+        LOG.warn("ProcedureExecutor not running, may have been stopped by pending procedure due to"
+            + " KillAndToggleBeforeStoreUpdate flag.");
+        ProcedureTestingUtility.setKillAndToggleBeforeStoreUpdate(procExec, false);
+        restartMasterProcedureExecutor(procExec);
+        ProcedureTestingUtility.waitNoProcedureRunning(procExec);
+      }
+    }
+
     assertEquals(true, procExec.isRunning());
     ProcedureTestingUtility.assertIsAbortException(procExec.getResult(procId));
   }