HBASE-23727 Port HBASE-20981 in 2.2 & 2.3

HBASE-20981 - Rollback stateCount accounting thrown-off when exception out of rollbackState

Signed-off-by: Michael Stack <stack@apache.org>
Signed-off-by: Sakthi <sakthi@apache.org>
Signed-off-by: Peter Somogyi <psomogyi@apache.org>

hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/StateMachineProcedure.java

@@ -54,7 +54,7 @@ public abstract class StateMachineProcedure<TEnvironment, TState>
   private final AtomicBoolean aborted = new AtomicBoolean(false);
 
   private Flow stateFlow = Flow.HAS_MORE_STATE;
-  private int stateCount = 0;
+  protected int stateCount = 0;
   private int[] states = null;
 
   private List<Procedure<TEnvironment>> subProcList = null;
@@ -217,13 +217,13 @@ public abstract class StateMachineProcedure<TEnvironment, TState>
     try {
       updateTimestamp();
       rollbackState(env, getCurrentState());
-      stateCount--;
     } finally {
+      stateCount--;
       updateTimestamp();
     }
   }
 
-  private boolean isEofState() {
+  protected boolean isEofState() {
     return stateCount > 0 && states[stateCount-1] == EOF_STATE;
   }
 

hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestStateMachineProcedure.java

@@ -149,6 +149,24 @@ public class TestStateMachineProcedure {
     assertEquals(TEST_FAILURE_EXCEPTION, cause);
   }
 
+  @Test
+  public void testChildNormalRollbackStateCount() {
+    procExecutor.getEnvironment().triggerChildRollback = true;
+    TestSMProcedureBadRollback testNormalRollback = new TestSMProcedureBadRollback();
+    long procId = procExecutor.submitProcedure(testNormalRollback);
+    ProcedureTestingUtility.waitProcedure(procExecutor, procId);
+    assertEquals(0, testNormalRollback.stateCount);
+  }
+
+  @Test
+  public void testChildBadRollbackStateCount() {
+    procExecutor.getEnvironment().triggerChildRollback = true;
+    TestSMProcedureBadRollback testBadRollback = new TestSMProcedureBadRollback();
+    long procId = procExecutor.submitProcedure(testBadRollback);
+    ProcedureTestingUtility.waitProcedure(procExecutor, procId);
+    assertEquals(0, testBadRollback.stateCount);
+  }
+
   @Test
   public void testChildOnLastStepWithRollbackDoubleExecution() throws Exception {
     procExecutor.getEnvironment().triggerChildRollback = true;
@@ -208,6 +226,64 @@ public class TestStateMachineProcedure {
     }
   }
 
+  public static class TestSMProcedureBadRollback
+          extends StateMachineProcedure<TestProcEnv, TestSMProcedureState> {
+    @Override
+    protected Flow executeFromState(TestProcEnv env, TestSMProcedureState state) {
+      LOG.info("EXEC " + state + " " + this);
+      env.execCount.incrementAndGet();
+      switch (state) {
+        case STEP_1:
+          if (!env.loop) {
+            setNextState(TestSMProcedureState.STEP_2);
+          }
+          break;
+        case STEP_2:
+          addChildProcedure(new SimpleChildProcedure());
+          return Flow.NO_MORE_STATE;
+      }
+      return Flow.HAS_MORE_STATE;
+    }
+    @Override
+    protected void rollbackState(TestProcEnv env, TestSMProcedureState state) {
+      LOG.info("ROLLBACK " + state + " " + this);
+      env.rollbackCount.incrementAndGet();
+    }
+
+    @Override
+    protected TestSMProcedureState getState(int stateId) {
+      return TestSMProcedureState.values()[stateId];
+    }
+
+    @Override
+    protected int getStateId(TestSMProcedureState state) {
+      return state.ordinal();
+    }
+
+    @Override
+    protected TestSMProcedureState getInitialState() {
+      return TestSMProcedureState.STEP_1;
+    }
+
+    @Override
+    protected void rollback(final TestProcEnv env)
+            throws IOException, InterruptedException {
+      if (isEofState()) {
+        stateCount--;
+      }
+      try {
+        updateTimestamp();
+        rollbackState(env, getCurrentState());
+        throw new IOException();
+      } catch(IOException e) {
+        //do nothing for now
+      } finally {
+        stateCount--;
+        updateTimestamp();
+      }
+    }
+  }
+
   public static class SimpleChildProcedure extends NoopProcedure<TestProcEnv> {
     @Override
     protected Procedure<TestProcEnv>[] execute(TestProcEnv env) {

hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestYieldProcedures.java

@@ -142,15 +142,17 @@ public class TestYieldProcedures {
       assertEquals(i, info.getStep().ordinal());
     }
 
-    // test rollback (we execute steps twice, one has the IE the other completes)
+    // test rollback (we execute steps twice, rollback counts both IE and completed)
     for (int i = NUM_STATES - 1; i >= 0; --i) {
       TestStateMachineProcedure.ExecutionInfo info = proc.getExecutionInfo().get(count++);
       assertEquals(true, info.isRollback());
       assertEquals(i, info.getStep().ordinal());
+    }
 
-      info = proc.getExecutionInfo().get(count++);
+    for (int i = NUM_STATES - 1; i >= 0; --i) {
+      TestStateMachineProcedure.ExecutionInfo info = proc.getExecutionInfo().get(count++);
       assertEquals(true, info.isRollback());
-      assertEquals(i, info.getStep().ordinal());
+      assertEquals(0, info.getStep().ordinal());
     }
 
     // check runnable queue stats