From 7da9e169fb9d6c4b68012eba2e14807e8826e698 Mon Sep 17 00:00:00 2001 From: zhangduo Date: Thu, 20 Sep 2018 21:43:30 +0800 Subject: [PATCH] HBASE-21199 Race in region opening and load balancing can cause region stuck in RIT --- .../apache/hadoop/hbase/master/HMaster.java | 8 +- .../hbase/regionserver/HRegionServer.java | 38 ++--- .../handler/OpenRegionHandler.java | 19 +-- .../master/TestCloseAnOpeningRegion.java | 146 ++++++++++++++++++ 4 files changed, 175 insertions(+), 36 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestCloseAnOpeningRegion.java 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 ad7c355357..8c98bbc297 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 @@ -846,6 +846,12 @@ public class HMaster extends HRegionServer implements MasterServices { } } + // Will be overriden in test to inject customized AssignmentManager + @VisibleForTesting + protected AssignmentManager createAssignmentManager(MasterServices master) { + return new AssignmentManager(master); + } + /** * Finish initialization of HMaster after becoming the primary master. *

@@ -938,7 +944,7 @@ public class HMaster extends HRegionServer implements MasterServices { checkUnsupportedProcedure(procsByType); // Create Assignment Manager - this.assignmentManager = new AssignmentManager(this); + this.assignmentManager = createAssignmentManager(this); this.assignmentManager.start(); // TODO: TRSP can perform as the sub procedure for other procedures, so even if it is marked as // completed, it could still be in the procedure list. This is a bit strange but is another diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java index 76175117af..1b6f319079 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java @@ -3154,29 +3154,21 @@ public class HRegionServer extends HasThread implements final Boolean previous = this.regionsInTransitionInRS.putIfAbsent(Bytes.toBytes(encodedName), Boolean.FALSE); - - if (Boolean.TRUE.equals(previous)) { - LOG.info("Received CLOSE for the region:" + encodedName + " , which we are already " + - "trying to OPEN. Cancelling OPENING."); - if (!regionsInTransitionInRS.replace(Bytes.toBytes(encodedName), previous, Boolean.FALSE)) { - // The replace failed. That should be an exceptional case, but theoretically it can happen. - // We're going to try to do a standard close then. - LOG.warn("The opening for region " + encodedName + " was done before we could cancel it." + - " Doing a standard close now"); - return closeRegion(encodedName, abort, sn); - } - // Let's get the region from the online region list again - actualRegion = this.getRegion(encodedName); - if (actualRegion == null) { // If already online, we still need to close it. - LOG.info("The opening previously in progress has been cancelled by a CLOSE request."); - // The master deletes the znode when it receives this exception. - throw new NotServingRegionException("The region " + encodedName + - " was opening but not yet served. Opening is cancelled."); - } - } else if (Boolean.FALSE.equals(previous)) { - LOG.info("Received CLOSE for the region: " + encodedName + - ", which we are already trying to CLOSE, but not completed yet"); - return true; + if (previous != null) { + if (previous) { + // This could happen as we will update the region state to OPEN when calling + // reportRegionStateTransition, so the HMaster will think the region is online, before we + // actually open the region, as the This could happen we will update the region state to + // OPEN when calling reportRegionStateTransition is part of the opening process. + LOG.warn("Received CLOSE for the region:" + encodedName + " , which we are already " + + "trying to OPEN. Please try again later."); + throw new RegionOpeningException( + "The region " + encodedName + " is opening currently, please try again later."); + } else { + LOG.info("Received CLOSE for the region: " + encodedName + + ", which we are already trying to CLOSE, but not completed yet"); + return true; + } } if (actualRegion == null) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java index 970911fde7..cb0d7881ad 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java @@ -133,20 +133,15 @@ public class OpenRegionHandler extends EventHandler { final Boolean current = this.rsServices.getRegionsInTransitionInRS(). remove(this.regionInfo.getEncodedNameAsBytes()); - // Let's check if we have met a race condition on open cancellation.... - // A better solution would be to not have any race condition. - // this.rsServices.getRegionsInTransitionInRS().remove( - // this.regionInfo.getEncodedNameAsBytes(), Boolean.TRUE); - // would help. + // Let's check if we have met a race condition, theoretically this should not happen. if (openSuccessful) { if (current == null) { // Should NEVER happen, but let's be paranoid. - LOG.error("Bad state: we've just opened a region that was NOT in transition. Region=" - + regionName); - } else if (Boolean.FALSE.equals(current)) { // Can happen, if we're - // really unlucky. - LOG.error("Race condition: we've finished to open a region, while a close was requested " - + " on region=" + regionName + ". It can be a critical error, as a region that" - + " should be closed is now opened. Closing it now"); + LOG.error("Bad state: we've just opened a region that was NOT in transition. Region=" + + regionName); + } else if (!current) { + // Should NEVER happen too. For now if we are closing a region which is current being + // opening, we will throw exception and try again later. But let's be paranoid. + LOG.error("Bad state: we've just opened a region that was closing. Region=" + regionName); cleanupFailedOpen(region); } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestCloseAnOpeningRegion.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestCloseAnOpeningRegion.java new file mode 100644 index 0000000000..bbf991c95e --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestCloseAnOpeningRegion.java @@ -0,0 +1,146 @@ +/** + * 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; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.util.concurrent.CountDownLatch; +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.PleaseHoldException; +import org.apache.hadoop.hbase.StartMiniClusterOption; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Put; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.Table; +import org.apache.hadoop.hbase.master.assignment.AssignmentManager; +import org.apache.hadoop.hbase.regionserver.HRegionServer; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.zookeeper.KeeperException; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.RegionStateTransition.TransitionCode; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.ReportRegionStateTransitionRequest; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.ReportRegionStateTransitionResponse; + +@Category({ MasterTests.class, MediumTests.class }) +public class TestCloseAnOpeningRegion { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestCloseAnOpeningRegion.class); + + private static final HBaseTestingUtility UTIL = new HBaseTestingUtility(); + + private static TableName TABLE_NAME = TableName.valueOf("race"); + + private static byte[] CF = Bytes.toBytes("cf"); + + private static volatile CountDownLatch ARRIVE; + + private static volatile CountDownLatch RESUME; + + public static final class MockHMaster extends HMaster { + + public MockHMaster(Configuration conf) throws IOException, KeeperException { + super(conf); + } + + @Override + protected AssignmentManager createAssignmentManager(MasterServices master) { + return new AssignmentManager(master) { + + @Override + public ReportRegionStateTransitionResponse reportRegionStateTransition( + ReportRegionStateTransitionRequest req) throws PleaseHoldException { + ReportRegionStateTransitionResponse resp = super.reportRegionStateTransition(req); + TransitionCode code = req.getTransition(0).getTransitionCode(); + if (code == TransitionCode.OPENED && ARRIVE != null) { + ARRIVE.countDown(); + try { + RESUME.await(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } + return resp; + } + }; + } + } + + @BeforeClass + public static void setUp() throws Exception { + UTIL.getConfiguration().setInt(HConstants.HBASE_RPC_SHORTOPERATION_TIMEOUT_KEY, 60000); + UTIL.startMiniCluster( + StartMiniClusterOption.builder().numRegionServers(2).masterClass(MockHMaster.class).build()); + UTIL.createTable(TABLE_NAME, CF); + UTIL.getAdmin().balancerSwitch(false, true); + } + + @AfterClass + public static void tearDown() throws Exception { + UTIL.shutdownMiniCluster(); + } + + @Test + public void test() throws IOException, InterruptedException { + ARRIVE = new CountDownLatch(1); + RESUME = new CountDownLatch(1); + RegionInfo region = UTIL.getAdmin().getRegions(TABLE_NAME).get(0); + HRegionServer src = UTIL.getRSForFirstRegionInTable(TABLE_NAME); + HRegionServer dst = UTIL.getOtherRegionServer(src); + Thread move0 = new Thread(() -> { + try { + UTIL.getAdmin().move(region.getEncodedNameAsBytes(), + Bytes.toBytes(dst.getServerName().getServerName())); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + }); + move0.start(); + ARRIVE.await(); + Thread move1 = new Thread(() -> { + try { + UTIL.getAdmin().move(region.getEncodedNameAsBytes(), + Bytes.toBytes(src.getServerName().getServerName())); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + }); + move1.start(); + // No simple way to determine when it is safe to go on and produce the race so let's sleep for a + // well... + Thread.sleep(10000); + RESUME.countDown(); + move0.join(); + move1.join(); + try (Table table = UTIL.getConnection().getTable(TABLE_NAME)) { + // make sure that we can write to the table, which means the region is online + table.put(new Put(Bytes.toBytes(0)).addColumn(CF, Bytes.toBytes("cq"), Bytes.toBytes(0))); + } + } +} -- 2.17.1