From 3a0f72283009e279379da353254dbafa1a9ec172 Mon Sep 17 00:00:00 2001 From: zhangduo Date: Fri, 21 Sep 2018 09:44:48 +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 ++++++++++++++++++ .../TestRegionServerNoMaster.java | 69 ++------- 5 files changed, 192 insertions(+), 88 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))); + } + } +} diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerNoMaster.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerNoMaster.java index af2861fe65..21f0c532af 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerNoMaster.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerNoMaster.java @@ -21,16 +21,15 @@ import java.io.IOException; import org.apache.hadoop.hbase.HBaseClassTestRule; import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.HConstants; -import org.apache.hadoop.hbase.HRegionInfo; import org.apache.hadoop.hbase.NotServingRegionException; import org.apache.hadoop.hbase.ServerName; 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.RegionInfoBuilder; import org.apache.hadoop.hbase.client.RegionLocator; import org.apache.hadoop.hbase.client.Table; -import org.apache.hadoop.hbase.client.TableDescriptor; import org.apache.hadoop.hbase.master.HMaster; -import org.apache.hadoop.hbase.regionserver.handler.OpenRegionHandler; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.testclassification.RegionServerTests; import org.apache.hadoop.hbase.util.Bytes; @@ -67,7 +66,7 @@ public class TestRegionServerNoMaster { private static Table table; private static final byte[] row = Bytes.toBytes("ee"); - private static HRegionInfo hri; + private static RegionInfo hri; private static byte[] regionName; private static final HBaseTestingUtility HTU = new HBaseTestingUtility(); @@ -85,7 +84,7 @@ public class TestRegionServerNoMaster { table.put(p); try (RegionLocator locator = HTU.getConnection().getRegionLocator(tableName)) { - hri = locator.getRegionLocation(row, false).getRegionInfo(); + hri = locator.getRegionLocation(row, false).getRegion(); } regionName = hri.getRegionName(); @@ -117,21 +116,23 @@ public class TestRegionServerNoMaster { return; } - ProtobufUtil.openRegion(null, hrs.getRSRpcServices(), - hrs.getServerName(), HRegionInfo.FIRST_META_REGIONINFO); + ProtobufUtil.openRegion(null, hrs.getRSRpcServices(), hrs.getServerName(), + RegionInfoBuilder.FIRST_META_REGIONINFO); while (true) { sn = mtl.getMetaRegionLocation(zkw); - if (sn != null && sn.equals(hrs.getServerName()) - && hrs.onlineRegions.containsKey( - HRegionInfo.FIRST_META_REGIONINFO.getEncodedName())) { + if (sn != null && sn.equals(hrs.getServerName()) && + hrs.onlineRegions.containsKey(RegionInfoBuilder.FIRST_META_REGIONINFO.getEncodedName())) { break; } Thread.sleep(100); } } - /** Flush the given region in the mini cluster. Since no master, we cannot use HBaseAdmin.flush() */ - public static void flushRegion(HBaseTestingUtility HTU, HRegionInfo regionInfo) throws IOException { + /** + * Flush the given region in the mini cluster. Since no master, we cannot use HBaseAdmin.flush() + */ + public static void flushRegion(HBaseTestingUtility HTU, RegionInfo regionInfo) + throws IOException { for (RegionServerThread rst : HTU.getMiniHBaseCluster().getRegionServerThreads()) { HRegion region = rst.getRegionServer().getRegionByEncodedName(regionInfo.getEncodedName()); if (region != null) { @@ -154,7 +155,7 @@ public class TestRegionServerNoMaster { } - public static void openRegion(HBaseTestingUtility HTU, HRegionServer rs, HRegionInfo hri) + public static void openRegion(HBaseTestingUtility HTU, HRegionServer rs, RegionInfo hri) throws Exception { AdminProtos.OpenRegionRequest orr = RequestConverter.buildOpenRegionRequest(rs.getServerName(), hri, null); @@ -169,7 +170,7 @@ public class TestRegionServerNoMaster { } public static void checkRegionIsOpened(HBaseTestingUtility HTU, HRegionServer rs, - HRegionInfo hri) throws Exception { + RegionInfo hri) throws Exception { while (!rs.getRegionsInTransitionInRS().isEmpty()) { Thread.sleep(1); } @@ -177,7 +178,7 @@ public class TestRegionServerNoMaster { Assert.assertTrue(rs.getRegion(hri.getRegionName()).isAvailable()); } - public static void closeRegion(HBaseTestingUtility HTU, HRegionServer rs, HRegionInfo hri) + public static void closeRegion(HBaseTestingUtility HTU, HRegionServer rs, RegionInfo hri) throws Exception { AdminProtos.CloseRegionRequest crr = ProtobufUtil.buildCloseRegionRequest( rs.getServerName(), hri.getRegionName()); @@ -187,7 +188,7 @@ public class TestRegionServerNoMaster { } public static void checkRegionIsClosed(HBaseTestingUtility HTU, HRegionServer rs, - HRegionInfo hri) throws Exception { + RegionInfo hri) throws Exception { while (!rs.getRegionsInTransitionInRS().isEmpty()) { Thread.sleep(1); } @@ -239,42 +240,6 @@ public class TestRegionServerNoMaster { openRegion(HTU, getRS(), hri); } - /** - * Test that if we do a close while opening it stops the opening. - */ - @Test - public void testCancelOpeningWithoutZK() throws Exception { - // We close - closeRegionNoZK(); - checkRegionIsClosed(HTU, getRS(), hri); - - // Let do the initial steps, without having a handler - getRS().getRegionsInTransitionInRS().put(hri.getEncodedNameAsBytes(), Boolean.TRUE); - - // That's a close without ZK. - AdminProtos.CloseRegionRequest crr = - ProtobufUtil.buildCloseRegionRequest(getRS().getServerName(), regionName); - try { - getRS().rpcServices.closeRegion(null, crr); - Assert.assertTrue(false); - } catch (org.apache.hbase.thirdparty.com.google.protobuf.ServiceException expected) { - } - - // The state in RIT should have changed to close - Assert.assertEquals(Boolean.FALSE, getRS().getRegionsInTransitionInRS().get( - hri.getEncodedNameAsBytes())); - - // Let's start the open handler - TableDescriptor htd = getRS().tableDescriptors.get(hri.getTable()); - - getRS().executorService.submit(new OpenRegionHandler(getRS(), getRS(), hri, htd, -1)); - - // The open handler should have removed the region from RIT but kept the region closed - checkRegionIsClosed(HTU, getRS(), hri); - - openRegion(HTU, getRS(), hri); - } - /** * Tests an on-the-fly RPC that was scheduled for the earlier RS on the same port * for openRegion. The region server should reject this RPC. (HBASE-9721) -- 2.17.1