From 4215794f4707da4d667608ef2af99e2a47231b30 Mon Sep 17 00:00:00 2001 From: zhangduo Date: Sat, 13 Oct 2018 17:24:23 +0800 Subject: [PATCH] HBASE-21278 Do not rollback successful sub procedures when rolling back a procedure --- .../hbase/procedure2/ProcedureExecutor.java | 60 ++++++++++++------- .../procedure2/StateMachineProcedure.java | 14 +++-- .../assignment/AssignmentManagerUtil.java | 45 ++++++++++---- .../TestMergeTableRegionsProcedure.java | 22 +++---- .../TestSplitTableRegionProcedure.java | 17 +++--- .../MasterProcedureTestingUtility.java | 9 ++- 6 files changed, 107 insertions(+), 60 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 7b5ab73c87..9412fbdbda 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 @@ -1594,7 +1594,19 @@ public class ProcedureExecutor { int stackTail = subprocStack.size(); while (stackTail-- > 0) { Procedure proc = subprocStack.get(stackTail); - + // For the sub procedures which are successfully finished, we do not rollback them. + // Typically, if we want to rollback a procedure, we first need to rollback it, and then + // recursively rollback its ancestors. The state changes which are done by sub procedures + // should be handled by parent procedures when rolling back. For example, when rolling back a + // MergeTableProcedure, we will schedule new procedures to bring the offline regions online, + // instead of rolling back the original procedures which offlined the regions(in fact these + // procedures can not be rolled back...). + if (proc.isSuccess()) { + // Just do the cleanup work, without actually executing the rollback + subprocStack.remove(stackTail); + cleanupAfterRollbackOneStep(proc); + continue; + } LockState lockState = acquireLock(proc); if (lockState != LockState.LOCK_ACQUIRED) { // can't take a lock on the procedure, add the root-proc back on the @@ -1633,6 +1645,31 @@ public class ProcedureExecutor { return LockState.LOCK_ACQUIRED; } + private void cleanupAfterRollbackOneStep(Procedure proc) { + if (proc.removeStackIndex()) { + if (!proc.isSuccess()) { + proc.setState(ProcedureState.ROLLEDBACK); + } + + // update metrics on finishing the procedure (fail) + proc.updateMetricsOnFinish(getEnvironment(), proc.elapsedTime(), false); + + if (proc.hasParent()) { + store.delete(proc.getProcId()); + procedures.remove(proc.getProcId()); + } else { + final long[] childProcIds = rollbackStack.get(proc.getProcId()).getSubprocedureIds(); + if (childProcIds != null) { + store.delete(proc, childProcIds); + } else { + store.update(proc); + } + } + } else { + store.update(proc); + } + } + /** * Execute the rollback of the procedure step. * It updates the store with the new state (stack index) @@ -1661,26 +1698,7 @@ public class ProcedureExecutor { throw new RuntimeException(msg); } - if (proc.removeStackIndex()) { - proc.setState(ProcedureState.ROLLEDBACK); - - // update metrics on finishing the procedure (fail) - proc.updateMetricsOnFinish(getEnvironment(), proc.elapsedTime(), false); - - if (proc.hasParent()) { - store.delete(proc.getProcId()); - procedures.remove(proc.getProcId()); - } else { - final long[] childProcIds = rollbackStack.get(proc.getProcId()).getSubprocedureIds(); - if (childProcIds != null) { - store.delete(proc, childProcIds); - } else { - store.update(proc); - } - } - } else { - store.update(proc); - } + cleanupAfterRollbackOneStep(proc); return LockState.LOCK_ACQUIRED; } 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 986b250d9b..0a087a6a0d 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 @@ -219,12 +219,16 @@ public abstract class StateMachineProcedure @Override protected boolean abort(final TEnvironment env) { LOG.debug("Abort requested for {}", this); - if (hasMoreState()) { - aborted.set(true); - return true; + if (!hasMoreState()) { + LOG.warn("Ignore abort request on {} because it has already been finished", this); + return false; } - LOG.debug("Ignoring abort request on {}", this); - return false; + if (!isRollbackSupported(getCurrentState())) { + LOG.warn("Ignore abort request on {} because it does not support rollback", this); + return false; + } + aborted.set(true); + return true; } /** diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java index 7f2d11a82e..36ac6cfb3d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java @@ -140,26 +140,45 @@ final class AssignmentManagerUtil { return procs; } + /** + * Create assign procedures for the give regions, according to the {@code regionReplication}. + *

+ * For rolling back, we will submit procedures directly to the {@code ProcedureExecutor}, so it is + * possible that we persist the newly scheduled procedures, and then crash before persisting the + * rollback state, so when we arrive here the second time, it is possible that some regions have + * already been associated with a TRSP. + * @param ignoreIfInTransition if true, will skip creating TRSP for the given region if it is + * already in transition, otherwise we will add an assert that it should not in + * transition. + */ private static TransitRegionStateProcedure[] createAssignProcedures(MasterProcedureEnv env, - List regions, int regionReplication, ServerName targetServer) { + List regions, int regionReplication, ServerName targetServer, + boolean ignoreIfInTransition) { // create the assign procs only for the primary region using the targetServer - TransitRegionStateProcedure[] primaryRegionProcs = regions.stream() - .map(env.getAssignmentManager().getRegionStates()::getOrCreateRegionStateNode) + TransitRegionStateProcedure[] primaryRegionProcs = + regions.stream().map(env.getAssignmentManager().getRegionStates()::getOrCreateRegionStateNode) .map(regionNode -> { TransitRegionStateProcedure proc = - TransitRegionStateProcedure.assign(env, regionNode.getRegionInfo(), targetServer); + TransitRegionStateProcedure.assign(env, regionNode.getRegionInfo(), targetServer); regionNode.lock(); try { - // should never fail, as we have the exclusive region lock, and the region is newly - // created, or has been successfully closed so should not be on any servers, so SCP will - // not process it either. - assert !regionNode.isInTransition(); + if (ignoreIfInTransition) { + if (regionNode.isInTransition()) { + return null; + } + } else { + // should never fail, as we have the exclusive region lock, and the region is newly + // created, or has been successfully closed so should not be on any servers, so SCP + // will + // not process it either. + assert !regionNode.isInTransition(); + } regionNode.setProcedure(proc); } finally { regionNode.unlock(); } return proc; - }).toArray(TransitRegionStateProcedure[]::new); + }).filter(p -> p != null).toArray(TransitRegionStateProcedure[]::new); if (regionReplication == DEFAULT_REGION_REPLICA) { // this is the default case return primaryRegionProcs; @@ -184,14 +203,16 @@ final class AssignmentManagerUtil { static TransitRegionStateProcedure[] createAssignProceduresForOpeningNewRegions( MasterProcedureEnv env, List regions, int regionReplication, ServerName targetServer) { - return createAssignProcedures(env, regions, regionReplication, targetServer); + return createAssignProcedures(env, regions, regionReplication, targetServer, false); } static void reopenRegionsForRollback(MasterProcedureEnv env, List regions, int regionReplication, ServerName targetServer) { TransitRegionStateProcedure[] procs = - createAssignProcedures(env, regions, regionReplication, targetServer); - env.getMasterServices().getMasterProcedureExecutor().submitProcedures(procs); + createAssignProcedures(env, regions, regionReplication, targetServer, true); + if (procs.length > 0) { + env.getMasterServices().getMasterProcedureExecutor().submitProcedures(procs); + } } static void removeNonDefaultReplicas(MasterProcedureEnv env, Stream regions, diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMergeTableRegionsProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMergeTableRegionsProcedure.java index 36d7583caa..858d20ca9d 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMergeTableRegionsProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMergeTableRegionsProcedure.java @@ -24,7 +24,6 @@ import java.util.List; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HBaseTestingUtility; -import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.MetaTableAccessor; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.Admin; @@ -38,6 +37,7 @@ import org.apache.hadoop.hbase.master.procedure.MasterProcedureTestingUtility; import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; import org.apache.hadoop.hbase.procedure2.ProcedureMetrics; import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility; +import org.apache.hadoop.hbase.regionserver.HRegion; import org.apache.hadoop.hbase.testclassification.MasterTests; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; @@ -66,8 +66,6 @@ public class TestMergeTableRegionsProcedure { public final TestName name = new TestName(); private static final HBaseTestingUtility UTIL = new HBaseTestingUtility(); - private static long nonceGroup = HConstants.NO_NONCE; - private static long nonce = HConstants.NO_NONCE; private static final int initialRegionCount = 4; private final static byte[] FAMILY = Bytes.toBytes("FAMILY"); @@ -108,9 +106,8 @@ public class TestMergeTableRegionsProcedure { @Before public void setup() throws Exception { resetProcExecutorTestingKillFlag(); - nonceGroup = - MasterProcedureTestingUtility.generateNonceGroup(UTIL.getHBaseCluster().getMaster()); - nonce = MasterProcedureTestingUtility.generateNonce(UTIL.getHBaseCluster().getMaster()); + MasterProcedureTestingUtility.generateNonceGroup(UTIL.getHBaseCluster().getMaster()); + MasterProcedureTestingUtility.generateNonce(UTIL.getHBaseCluster().getMaster()); // Turn off balancer so it doesn't cut in and mess up our placements. admin.balancerSwitch(false, true); // Turn off the meta scanner so it don't remove parent on us. @@ -265,12 +262,15 @@ public class TestMergeTableRegionsProcedure { long procId = procExec.submitProcedure( new MergeTableRegionsProcedure(procExec.getEnvironment(), regionsToMerge, true)); - // Failing before MERGE_TABLE_REGIONS_CREATE_MERGED_REGION we should trigger the rollback - // NOTE: the 5 (number before MERGE_TABLE_REGIONS_CREATE_MERGED_REGION step) is + // Failing before MERGE_TABLE_REGIONS_UPDATE_META we should trigger the rollback + // NOTE: the 8 (number of MERGE_TABLE_REGIONS_UPDATE_META 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, - true); + int lastStep = 8; + MasterProcedureTestingUtility.testRollbackAndDoubleExecution(procExec, procId, lastStep, true); + assertEquals(initialRegionCount, UTIL.getAdmin().getRegions(tableName).size()); + UTIL.waitUntilAllRegionsAssigned(tableName); + List regions = UTIL.getMiniHBaseCluster().getRegions(tableName); + assertEquals(initialRegionCount, regions.size()); } @Test diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestSplitTableRegionProcedure.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestSplitTableRegionProcedure.java index b9fb518021..3f9f960b92 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestSplitTableRegionProcedure.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestSplitTableRegionProcedure.java @@ -348,7 +348,7 @@ public class TestSplitTableRegionProcedure { final TableName tableName = TableName.valueOf(name.getMethodName()); final ProcedureExecutor procExec = getMasterProcedureExecutor(); - RegionInfo [] regions = MasterProcedureTestingUtility.createTable( + RegionInfo[] regions = MasterProcedureTestingUtility.createTable( procExec, tableName, null, ColumnFamilyName1, ColumnFamilyName2); insertData(tableName); int splitRowNum = startRowNum + rowCount / 2; @@ -366,18 +366,19 @@ public class TestSplitTableRegionProcedure { long procId = procExec.submitProcedure( new SplitTableRegionProcedure(procExec.getEnvironment(), regions[0], splitKey)); - // Failing before SPLIT_TABLE_REGION_CREATE_DAUGHTER_REGIONS we should trigger the + // Failing before SPLIT_TABLE_REGION_UPDATE_META we should trigger the // rollback - // NOTE: the 3 (number before SPLIT_TABLE_REGION_CREATE_DAUGHTER_REGIONS step) is + // NOTE: the 7 (number of SPLIT_TABLE_REGION_UPDATE_META 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, + int lastStep = 7; + MasterProcedureTestingUtility.testRollbackAndDoubleExecution(procExec, procId, lastStep, true); // check that we have only 1 region assertEquals(1, UTIL.getAdmin().getRegions(tableName).size()); - List daughters = UTIL.getMiniHBaseCluster().getRegions(tableName); - assertEquals(1, daughters.size()); - verifyData(daughters.get(0), startRowNum, rowCount, + UTIL.waitUntilAllRegionsAssigned(tableName); + List newRegions = UTIL.getMiniHBaseCluster().getRegions(tableName); + assertEquals(1, newRegions.size()); + verifyData(newRegions.get(0), startRowNum, rowCount, Bytes.toBytes(ColumnFamilyName1), Bytes.toBytes(ColumnFamilyName2)); assertEquals(splitSubmittedCount + 1, splitProcMetrics.getSubmittedCounter().getCount()); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java index 3975ec0884..af4dd26104 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/MasterProcedureTestingUtility.java @@ -379,7 +379,7 @@ public class MasterProcedureTestingUtility { */ public static void testRecoveryAndDoubleExecution( final ProcedureExecutor procExec, final long procId, - final int numSteps, final boolean expectExecRunning) throws Exception { + final int lastStep, final boolean expectExecRunning) throws Exception { ProcedureTestingUtility.waitProcedure(procExec, procId); assertEquals(false, procExec.isRunning()); @@ -397,10 +397,13 @@ public class MasterProcedureTestingUtility { // fix would be get all visited states by the procedure and then check if user speccified // state is in that list. Current assumption of sequential proregression of steps/ states is // made at multiple places so we can keep while condition below for simplicity. - Procedure proc = procExec.getProcedure(procId); + Procedure proc = procExec.getProcedure(procId); int stepNum = proc instanceof StateMachineProcedure ? ((StateMachineProcedure) proc).getCurrentStateId() : 0; - while (stepNum < numSteps) { + for (;;) { + if (stepNum == lastStep) { + break; + } LOG.info("Restart " + stepNum + " exec state=" + proc); ProcedureTestingUtility.assertProcNotYetCompleted(procExec, procId); restartMasterProcedureExecutor(procExec); -- 2.17.1