From 29a00345bd4066e981873cb3fffbbecd87b2508b Mon Sep 17 00:00:00 2001 From: Michael Stack Date: Thu, 20 Sep 2018 16:53:58 -0700 Subject: [PATCH] HBASE-21213 [hbck2] Need more cleanup needed on bypass; old Procedure left in RegionStateNodes M hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java Have bypass take an environment like all other methods so subclasses Fix javadoc issues. M hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java Javadoc issues. Pass environment when we invoke bypass. M hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java Rename waitUntilNamespace... etc. to align with how these method types are named elsehwere .. i.e. waitFor rather than waitUntil.. M hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionTransitionProcedure.java Cleanup message we emit when we find an exisitng procedure working against this entity. Override bypass. Call cleanup method. A hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionBypass.java Test bypass clears up state such that a subsequent assign can work. M hbase-shell/src/main/ruby/shell/commands/list_procedures.rb Minor cleanup of the json output... do iso8601 timestamps. --- .../java/org/apache/hadoop/hbase/client/Hbck.java | 5 +- .../apache/hadoop/hbase/procedure2/Procedure.java | 15 +- .../hadoop/hbase/procedure2/ProcedureExecutor.java | 13 +- .../hadoop/hbase/procedure2/ProcedureUtil.java | 2 +- .../hbase/procedure2/TestProcedureBypass.java | 3 - .../org/apache/hadoop/hbase/master/HMaster.java | 8 +- .../assignment/RegionTransitionProcedure.java | 29 +++- .../apache/hadoop/hbase/TestMetaTableAccessor.java | 4 +- .../hbase/master/assignment/TestRegionBypass.java | 166 +++++++++++++++++++++ .../main/ruby/shell/commands/list_procedures.rb | 7 +- 10 files changed, 220 insertions(+), 32 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionBypass.java diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Hbck.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Hbck.java index 5c9a862feb..81638b408b 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Hbck.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Hbck.java @@ -28,7 +28,10 @@ import org.apache.yetus.audience.InterfaceAudience; /** * Hbck fixup tool APIs. Obtain an instance from {@link ClusterConnection#getHbck()} and call * {@link #close()} when done. - *

WARNING: the below methods can damage the cluster. For experienced users only. + *

WARNING: the below methods can damage the cluster. It may leave the cluster in an + * indeterminate state, e.g. region not assigned, or some hdfs files left behind. After running + * any of the below, operators may have to do some clean up on hdfs or schedule some assign + * procedures to get regions back online. DO AT YOUR OWN RISK. For experienced users only. * * @see ConnectionFactory * @see ClusterConnection diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java index a832c783fe..9527f52517 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/Procedure.java @@ -161,12 +161,15 @@ public abstract class Procedure implements Comparable implements Comparable { private Configuration conf; /** - * Created in the {@link #start(int, boolean)} method. Destroyed in {@link #join()} (FIX! Doing + * Created in the {@link #init(int, boolean)} method. Destroyed in {@link #join()} (FIX! Doing * resource handling rather than observing in a #join is unexpected). * Overridden when we do the ProcedureTestingUtility.testRecoveryAndDoubleExecution trickery * (Should be ok). @@ -316,7 +316,7 @@ public class ProcedureExecutor { private ThreadGroup threadGroup; /** - * Created in the {@link #start(int, boolean)} method. Terminated in {@link #join()} (FIX! Doing + * Created in the {@link #init(int, boolean)} method. Terminated in {@link #join()} (FIX! Doing * resource handling rather than observing in a #join is unexpected). * Overridden when we do the ProcedureTestingUtility.testRecoveryAndDoubleExecution trickery * (Should be ok). @@ -324,7 +324,7 @@ public class ProcedureExecutor { private CopyOnWriteArrayList workerThreads; /** - * Created in the {@link #start(int, boolean)} method. Terminated in {@link #join()} (FIX! Doing + * Created in the {@link #init(int, boolean)} method. Terminated in {@link #join()} (FIX! Doing * resource handling rather than observing in a #join is unexpected). * Overridden when we do the ProcedureTestingUtility.testRecoveryAndDoubleExecution trickery * (Should be ok). @@ -968,7 +968,7 @@ public class ProcedureExecutor { * Bypass a procedure. If the procedure is set to bypass, all the logic in * execute/rollback will be ignored and it will return success, whatever. * It is used to recover buggy stuck procedures, releasing the lock resources - * and letting other procedures to run. Bypassing one procedure (and its ancestors will + * and letting other procedures run. Bypassing one procedure (and its ancestors will * be bypassed automatically) may leave the cluster in a middle state, e.g. region * not assigned, or some hdfs files left behind. After getting rid of those stuck procedures, * the operators may have to do some clean up on hdfs or schedule some assign procedures @@ -1010,7 +1010,7 @@ public class ProcedureExecutor { boolean bypassProcedure(long pid, long lockWait, boolean force) throws IOException { Procedure procedure = getProcedure(pid); if (procedure == null) { - LOG.debug("Procedure with id={} does not exist, skipping bypass", pid); + LOG.debug("Procedure pid={} does not exist, skipping bypass", pid); return false; } @@ -1043,6 +1043,7 @@ public class ProcedureExecutor { LOG.debug("Bypassing procedures in RUNNABLE, WAITING and WAITING_TIMEOUT states " + "(with no parent), {}", procedure); + // Question: how is the bypass done here? return false; } @@ -1052,7 +1053,7 @@ public class ProcedureExecutor { Procedure current = procedure; while (current != null) { LOG.debug("Bypassing {}", current); - current.bypass(); + current.bypass(getEnvironment()); store.update(procedure); long parentID = current.getParentProcId(); current = getProcedure(parentID); diff --git a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureUtil.java b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureUtil.java index 8c8746e84b..e64db6e7a6 100644 --- a/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureUtil.java +++ b/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureUtil.java @@ -271,7 +271,7 @@ public final class ProcedureUtil { } if (proto.getBypass()) { - proc.bypass(); + proc.bypass(null); } ProcedureStateSerializer serializer = null; diff --git a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestProcedureBypass.java b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestProcedureBypass.java index d58d57e76e..ee55a1400b 100644 --- a/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestProcedureBypass.java +++ b/hbase-procedure/src/test/java/org/apache/hadoop/hbase/procedure2/TestProcedureBypass.java @@ -179,7 +179,4 @@ public class TestProcedureBypass { } } } - - - } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index d7e57e8e2b..8fc57ed63b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -978,7 +978,7 @@ public class HMaster extends HRegionServer implements MasterServices { // as procedures run -- in particular SCPs for crashed servers... One should put up hbase:meta // if it is down. It may take a while to come online. So, wait here until meta if for sure // available. Thats what waitUntilMetaOnline does. - if (!waitUntilMetaOnline()) { + if (!waitForMetaOnline()) { return; } this.assignmentManager.joinCluster(); @@ -1010,7 +1010,7 @@ public class HMaster extends HRegionServer implements MasterServices { // Here we expect hbase:namespace to be online. See inside initClusterSchemaService. // TODO: Fix this. Namespace is a pain being a sort-of system table. Fold it in to hbase:meta. // isNamespace does like isMeta and waits until namespace is onlined before allowing progress. - if (!waitUntilNamespaceOnline()) { + if (!waitForNamespaceOnline()) { return; } status.setStatus("Starting cluster schema service"); @@ -1094,7 +1094,7 @@ public class HMaster extends HRegionServer implements MasterServices { * and we will hold here until operator intervention. */ @VisibleForTesting - public boolean waitUntilMetaOnline() throws InterruptedException { + public boolean waitForMetaOnline() throws InterruptedException { return isRegionOnline(RegionInfoBuilder.FIRST_META_REGIONINFO); } @@ -1135,7 +1135,7 @@ public class HMaster extends HRegionServer implements MasterServices { * @return True if namespace table is up/online. */ @VisibleForTesting - public boolean waitUntilNamespaceOnline() throws InterruptedException { + public boolean waitForNamespaceOnline() throws InterruptedException { List ris = this.assignmentManager.getRegionStates(). getRegionsOfTable(TableName.NAMESPACE_TABLE_NAME); if (ris.isEmpty()) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionTransitionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionTransitionProcedure.java index 3b0735e46e..dc7d3a7d77 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionTransitionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionTransitionProcedure.java @@ -308,10 +308,9 @@ public abstract class RegionTransitionProcedure final AssignmentManager am = env.getAssignmentManager(); final RegionStateNode regionNode = getRegionState(env); if (!am.addRegionInTransition(regionNode, this)) { - String msg = String.format( - "There is already another procedure running on this region this=%s owner=%s", - this, regionNode.getProcedure()); - LOG.warn(msg + " " + this + "; " + regionNode.toShortString()); + String msg = String.format("Another procedure owns this region; owner={%s}, this={%s}", + regionNode.toShortString(), this); + LOG.warn(msg); setAbortFailure(getClass().getSimpleName(), msg); return null; } @@ -354,7 +353,7 @@ public abstract class RegionTransitionProcedure // 3. wait assignment response. completion/failure LOG.debug("Finishing {}; {}", this, regionNode.toShortString()); finishTransition(env, regionNode); - am.removeRegionInTransition(regionNode, this); + cleanup(am, regionNode); return null; } } while (retry); @@ -373,6 +372,26 @@ public abstract class RegionTransitionProcedure return new Procedure[] {this}; } + @Override + protected void bypass(MasterProcedureEnv env) { + // See javadoc. env can be null but should never be null when this procedure is running. + // Allow for it nonetheless. + if (env != null) { + cleanup(env.getAssignmentManager(), getRegionState(env)); + } else { + LOG.warn("NULL environment! Should never happen."); + } + super.bypass(env); + } + + /** + * Cleanup done at the end of finish. Removes us as RIT from am. + * Should be called by {@link #bypass(MasterProcedureEnv)} too else we'll be stuck as RIT. + */ + private void cleanup(AssignmentManager am, RegionStateNode regionNode) { + am.removeRegionInTransition(regionNode, this); + } + /** * At end of timeout, wake ourselves up so we run again. */ diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessor.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessor.java index 2916cc410c..0ccf26e9c2 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessor.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestMetaTableAccessor.java @@ -108,7 +108,7 @@ public class TestMetaTableAccessor { @Test public void testIsMetaWhenAllHealthy() throws InterruptedException { HMaster m = UTIL.getMiniHBaseCluster().getMaster(); - assertTrue(m.waitUntilMetaOnline()); + assertTrue(m.waitForMetaOnline()); } @Test @@ -117,7 +117,7 @@ public class TestMetaTableAccessor { int index = UTIL.getMiniHBaseCluster().getServerWithMeta(); HRegionServer rsWithMeta = UTIL.getMiniHBaseCluster().getRegionServer(index); rsWithMeta.abort("TESTING"); - assertTrue(m.waitUntilMetaOnline()); + assertTrue(m.waitForMetaOnline()); } /** diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionBypass.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionBypass.java new file mode 100644 index 0000000000..2e91a32ce3 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionBypass.java @@ -0,0 +1,166 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.master.assignment; + +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv; +import org.apache.hadoop.hbase.procedure2.Procedure; +import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos; +import org.apache.hadoop.hbase.testclassification.LargeTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.CountDownLatch; + +import static org.junit.Assert.assertTrue; + +/** + * Tests bypass on a region assign/unassign + */ +@Category({LargeTests.class}) +public class TestRegionBypass { + private final static Logger LOG = LoggerFactory.getLogger(TestRegionBypass.class); + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestRegionBypass.class); + + @Rule + public TestName name = new TestName(); + + private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility(); + private TableName tableName; + + @BeforeClass + public static void startCluster() throws Exception { + TEST_UTIL.startMiniCluster(2); + } + + @AfterClass + public static void stopCluster() throws Exception { + TEST_UTIL.shutdownMiniCluster(); + } + + @Before + public void before() throws IOException { + this.tableName = TableName.valueOf(this.name.getMethodName()); + // Create a table. Has one region at least. + TEST_UTIL.createTable(this.tableName, Bytes.toBytes("cf")); + + } + + @Test + public void testBypass() throws IOException { + Admin admin = TEST_UTIL.getAdmin(); + List regions = admin.getRegions(this.tableName); + for (RegionInfo ri: regions) { + admin.unassign(ri.getRegionName(), false); + } + List pids = new ArrayList<>(regions.size()); + for (RegionInfo ri: regions) { + Procedure p = new StallingAssignProcedure(ri); + pids.add(TEST_UTIL.getHBaseCluster().getMaster().getMasterProcedureExecutor(). + submitProcedure(p)); + } + for (Long pid: pids) { + while (!TEST_UTIL.getHBaseCluster().getMaster().getMasterProcedureExecutor().isStarted(pid)) { + Thread.currentThread().yield(); + } + } + // Call bypass on all. We should be stuck in the dispatch at this stage. + List> ps = + TEST_UTIL.getHBaseCluster().getMaster().getMasterProcedureExecutor().getProcedures(); + for (Procedure p: ps) { + if (p instanceof StallingAssignProcedure) { + List bs = TEST_UTIL.getHbck(). + bypassProcedure(Arrays.asList(p.getProcId()), 0, false); + for (Boolean b: bs) { + LOG.info("BYPASSED {} {}", p.getProcId(), b); + } + } + } + // Countdown the latch so its not hanging out. + for (Procedure p: ps) { + if (p instanceof StallingAssignProcedure) { + ((StallingAssignProcedure)p).latch.countDown(); + } + } + // Try and assign. + for (RegionInfo ri: regions) { + admin.assign(ri.getRegionName()); + } + while (!TEST_UTIL.getHBaseCluster().getMaster().getMasterProcedureExecutor(). + getActiveProcIds().isEmpty()) { + Thread.currentThread().yield(); + } + for (RegionInfo ri: regions) { + assertTrue(ri.toString(), TEST_UTIL.getMiniHBaseCluster().getMaster().getAssignmentManager(). + getRegionStates().isRegionOnline(ri)); + } + } + + /** + * An AssignProcedure that Stalls just before the finish. + */ + public static class StallingAssignProcedure extends AssignProcedure { + public final CountDownLatch latch = new CountDownLatch(2); + + public StallingAssignProcedure() { + super(); + } + + public StallingAssignProcedure(RegionInfo regionInfo) { + super(regionInfo); + } + + @Override + void setTransitionState(MasterProcedureProtos.RegionTransitionState state) { + if (state == MasterProcedureProtos.RegionTransitionState.REGION_TRANSITION_DISPATCH) { + try { + LOG.info("LATCH2 {}", this.latch.getCount()); + this.latch.await(); + LOG.info("LATCH3 {}", this.latch.getCount()); + } catch (InterruptedException e) { + e.printStackTrace(); + } + } else if (state == MasterProcedureProtos.RegionTransitionState.REGION_TRANSITION_QUEUE) { + // Set latch. + LOG.info("LATCH1 {}", this.latch.getCount()); + this.latch.countDown(); + } + super.setTransitionState(state); + } + } +} diff --git a/hbase-shell/src/main/ruby/shell/commands/list_procedures.rb b/hbase-shell/src/main/ruby/shell/commands/list_procedures.rb index 68842a0410..ac39a26fd7 100644 --- a/hbase-shell/src/main/ruby/shell/commands/list_procedures.rb +++ b/hbase-shell/src/main/ruby/shell/commands/list_procedures.rb @@ -31,12 +31,11 @@ EOF end def command - formatter.header(%w[Id Name State Submitted_Time Last_Update Parameters]) - + formatter.header(%w[PID Name State Submitted_Time Last_Update Parameters]) list = JSON.parse(admin.list_procedures) list.each do |proc| - submitted_time = Time.at(Integer(proc['submittedTime']) / 1000).to_s - last_update = Time.at(Integer(proc['lastUpdate']) / 1000).to_s + submitted_time = Time.iso8601(Integer(proc['submittedTime']) / 1000).to_s + last_update = Time.iso8601(Integer(proc['lastUpdate']) / 1000).to_s formatter.row([proc['procId'], proc['className'], proc['state'], submitted_time, last_update, proc['stateMessage']]) end -- 2.16.3