Repository: hbase
Updated Branches:
  refs/heads/branch-2.1 873d9f508 -> 2d12e1ecf


HBASE-20981. Rollback stateCount accounting thrown-off when exception out of 
rollbackState
Signed-off-by: Michael Stack <st...@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/hbase/repo
Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/2d12e1ec
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/2d12e1ec
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/2d12e1ec

Branch: refs/heads/branch-2.1
Commit: 2d12e1ecf0f72e8460b22047b30887b1cbe6d820
Parents: 873d9f5
Author: jackbearden <j...@jackbearden.com>
Authored: Wed Aug 1 12:50:25 2018 -0700
Committer: Michael Stack <st...@apache.org>
Committed: Sat Aug 11 11:58:12 2018 -0700

----------------------------------------------------------------------
 .../hbase/procedure2/StateMachineProcedure.java |  6 +-
 .../procedure2/TestStateMachineProcedure.java   | 76 ++++++++++++++++++++
 .../hbase/procedure2/TestYieldProcedures.java   |  8 ++-
 3 files changed, 84 insertions(+), 6 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/hbase/blob/2d12e1ec/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/StateMachineProcedure.java
----------------------------------------------------------------------
diff --git 
a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/StateMachineProcedure.java
 
b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/StateMachineProcedure.java
index 69ecb29..4ed82f2 100644
--- 
a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/StateMachineProcedure.java
+++ 
b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/StateMachineProcedure.java
@@ -55,7 +55,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;
@@ -206,13 +206,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;
   }
 

http://git-wip-us.apache.org/repos/asf/hbase/blob/2d12e1ec/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestStateMachineProcedure.java
----------------------------------------------------------------------
diff --git 
a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestStateMachineProcedure.java
 
b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestStateMachineProcedure.java
index 07e4330..9160c90 100644
--- 
a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestStateMachineProcedure.java
+++ 
b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestStateMachineProcedure.java
@@ -142,6 +142,24 @@ public class TestStateMachineProcedure {
   }
 
   @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;
     ProcedureTestingUtility.setKillAndToggleBeforeStoreUpdate(procExecutor, 
true);
@@ -195,6 +213,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[] execute(TestProcEnv env) {

http://git-wip-us.apache.org/repos/asf/hbase/blob/2d12e1ec/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestYieldProcedures.java
----------------------------------------------------------------------
diff --git 
a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestYieldProcedures.java
 
b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestYieldProcedures.java
index 8d9b325..18d92ea 100644
--- 
a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestYieldProcedures.java
+++ 
b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestYieldProcedures.java
@@ -143,15 +143,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

Reply via email to