From 59e8b8e2ba4d403d042fe4cc02f8f9f80aad67af Mon Sep 17 00:00:00 2001 From: Umesh Agashe Date: Fri, 7 Apr 2017 14:01:37 -0700 Subject: [PATCH] HBASE-17863-addendum: Reverted the order of updateStoreOnExec() and store.isRunning() in execProcedure() Change-Id: I1c9d5ee264f4f593a6b2a09011853ab63693f677 --- .../hadoop/hbase/procedure2/ProcedureExecutor.java | 11 ++++++++--- .../hbase/procedure2/ProcedureTestingUtility.java | 2 +- .../procedure2/store/wal/TestWALProcedureStore.java | 3 +-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java index 8832637435b..43f5839438e 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java @@ -1373,12 +1373,17 @@ public class ProcedureExecutor { return; } - // if the store is not running we are aborting - if (!store.isRunning()) return; - + // TODO: The code here doesn't check if store is running before persisting to the store as + // it relies on the method call below to throw RuntimeException to wind up the stack and + // executor thread to stop. The statement following the method call below seems to check if + // store is not running, to prevent scheduling children procedures, re-execution or yield + // of this procedure. This may need more scrutiny and subsequent cleanup in future // Commit the transaction updateStoreOnExec(procStack, procedure, subprocs); + // if the store is not running we are aborting + if (!store.isRunning()) return; + // if the procedure is kind enough to pass the slot to someone else, yield if (procedure.isRunnable() && !suspended && procedure.isYieldAfterExecutionStep(getEnvironment())) { diff --git a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/ProcedureTestingUtility.java b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/ProcedureTestingUtility.java index 1f4244a2e63..dd3c8f40cef 100644 --- a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/ProcedureTestingUtility.java +++ b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/ProcedureTestingUtility.java @@ -408,7 +408,7 @@ public class ProcedureTestingUtility { addStackIndex(index); } - public void setFinishedState() { + public void setSuccessState() { setState(ProcedureState.SUCCESS); } diff --git a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/store/wal/TestWALProcedureStore.java b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/store/wal/TestWALProcedureStore.java index f8c34869b31..525a663f277 100644 --- a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/store/wal/TestWALProcedureStore.java +++ b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/store/wal/TestWALProcedureStore.java @@ -26,7 +26,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Comparator; import java.util.HashSet; -import java.util.List; import java.util.Set; import org.apache.commons.logging.Log; @@ -785,7 +784,7 @@ public class TestWALProcedureStore { // back to A a.addStackId(5); - a.setFinishedState(); + a.setSuccessState(); procStore.delete(a, new long[] { b.getProcId(), c.getProcId() }); restartAndAssert(3, 0, 1, 0); }