From 9e360b7a32f0e0510d5a4fd16900eaeaccb52d2d Mon Sep 17 00:00:00 2001 From: zhangduo Date: Sun, 11 Nov 2018 15:40:23 +0800 Subject: [PATCH] HBASE-21463 The checkOnlineRegionsReport can accidentally complete a TRSP --- .../hbase/procedure2/ProcedureExecutor.java | 5 + .../master/assignment/AssignmentManager.java | 161 ++++++--------- .../master/assignment/MockMasterServices.java | 12 +- .../TestReportOnlineRegionsRace.java | 188 ++++++++++++++++++ 4 files changed, 253 insertions(+), 113 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestReportOnlineRegionsRace.java 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 c18ca32a42..cbdb9b87ac 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 @@ -1915,6 +1915,11 @@ public class ProcedureExecutor { return completed.size(); } + @VisibleForTesting + public IdLock getProcExecutionLock() { + return procExecutionLock; + } + // ========================================================================== // Worker Thread // ========================================================================== diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java index 765ab6ba32..73615567c7 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java @@ -39,7 +39,6 @@ import org.apache.hadoop.hbase.PleaseHoldException; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.UnknownRegionException; -import org.apache.hadoop.hbase.YouAreDeadException; import org.apache.hadoop.hbase.client.DoNotRetryRegionException; import org.apache.hadoop.hbase.client.RegionInfo; import org.apache.hadoop.hbase.client.RegionInfoBuilder; @@ -162,7 +161,8 @@ public class AssignmentManager implements ServerListener { this(master, new RegionStateStore(master)); } - public AssignmentManager(final MasterServices master, final RegionStateStore stateStore) { + @VisibleForTesting + AssignmentManager(final MasterServices master, final RegionStateStore stateStore) { this.master = master; this.regionStateStore = stateStore; this.metrics = new MetricsAssignmentManager(); @@ -979,23 +979,26 @@ public class AssignmentManager implements ServerListener { // RS Status update (report online regions) helpers // ============================================================================================ /** - * the master will call this method when the RS send the regionServerReport(). - * the report will contains the "online regions". - * this method will check the the online regions against the in-memory state of the AM, - * if there is a mismatch we will try to fence out the RS with the assumption - * that something went wrong on the RS side. + * The master will call this method when the RS send the regionServerReport(). The report will + * contains the "online regions". This method will check the the online regions against the + * in-memory state of the AM, and we will log a warn message if there is a mismatch. This is + * because that there is no fencing between the reportRegionStateTransition method and + * regionServerReport method, so there could be race and introduce inconsistency here, but + * actually there is no problem. + *

+ * Please see HBASE-21421 and HBASE-21463 for more details. */ - public void reportOnlineRegions(final ServerName serverName, final Set regionNames) - throws YouAreDeadException { - if (!isRunning()) return; + public void reportOnlineRegions(ServerName serverName, Set regionNames) { + if (!isRunning()) { + return; + } if (LOG.isTraceEnabled()) { - LOG.trace("ReportOnlineRegions " + serverName + " regionCount=" + regionNames.size() + - ", metaLoaded=" + isMetaLoaded() + " " + - regionNames.stream().map(element -> Bytes.toStringBinary(element)). - collect(Collectors.toList())); + LOG.trace("ReportOnlineRegions {} regionCount={}, metaLoaded={} {}", serverName, + regionNames.size(), isMetaLoaded(), + regionNames.stream().map(Bytes::toStringBinary).collect(Collectors.toList())); } - final ServerStateNode serverNode = regionStates.getOrCreateServer(serverName); + ServerStateNode serverNode = regionStates.getOrCreateServer(serverName); synchronized (serverNode) { if (!serverNode.isInState(ServerState.ONLINE)) { @@ -1003,103 +1006,57 @@ public class AssignmentManager implements ServerListener { return; } } - if (regionNames.isEmpty()) { // nothing to do if we don't have regions - LOG.trace("no online region found on " + serverName); - } else if (!isMetaLoaded()) { - // if we are still on startup, discard the report unless is from someone holding meta - checkOnlineRegionsReportForMeta(serverNode, regionNames); - } else { - // The Heartbeat updates us of what regions are only. check and verify the state. - checkOnlineRegionsReport(serverNode, regionNames); + LOG.trace("no online region found on {}", serverName); + return; } + if (!isMetaLoaded()) { + // we are still on startup, skip checking + return; + } + // The Heartbeat tells us of what regions are on the region serve, check the state. + checkOnlineRegionsReport(serverNode, regionNames); // wake report event wakeServerReportEvent(serverNode); } - void checkOnlineRegionsReportForMeta(ServerStateNode serverNode, Set regionNames) { - try { - for (byte[] regionName : regionNames) { - final RegionInfo hri = getMetaRegionFromName(regionName); - if (hri == null) { - if (LOG.isTraceEnabled()) { - LOG.trace("Skip online report for region=" + Bytes.toStringBinary(regionName) + - " while meta is loading"); - } - continue; - } - - RegionStateNode regionNode = regionStates.getOrCreateRegionStateNode(hri); - LOG.info("META REPORTED: " + regionNode); - regionNode.lock(); - try { - if (!reportTransition(regionNode, serverNode, TransitionCode.OPENED, 0)) { - LOG.warn("META REPORTED but no procedure found (complete?); set location=" + - serverNode.getServerName()); - regionNode.setRegionLocation(serverNode.getServerName()); - } else if (LOG.isTraceEnabled()) { - LOG.trace("META REPORTED: " + regionNode); - } - } finally { - regionNode.unlock(); - } + // just check and output possible inconsistency, without actually doing anything + private void checkOnlineRegionsReport(ServerStateNode serverNode, Set regionNames) { + ServerName serverName = serverNode.getServerName(); + for (byte[] regionName : regionNames) { + if (!isRunning()) { + return; } - } catch (IOException e) { - ServerName serverName = serverNode.getServerName(); - LOG.warn("KILLING " + serverName + ": " + e.getMessage()); - killRegionServer(serverNode); - } - } - - void checkOnlineRegionsReport(final ServerStateNode serverNode, final Set regionNames) { - final ServerName serverName = serverNode.getServerName(); - try { - for (byte[] regionName: regionNames) { - if (!isRunning()) { - return; - } - final RegionStateNode regionNode = regionStates.getRegionStateNodeFromName(regionName); - if (regionNode == null) { - throw new UnexpectedStateException("Not online: " + Bytes.toStringBinary(regionName)); - } - regionNode.lock(); - try { - if (regionNode.isInState(State.OPENING, State.OPEN)) { - if (!regionNode.getRegionLocation().equals(serverName)) { - throw new UnexpectedStateException(regionNode.toString() + - " reported OPEN on server=" + serverName + - " but state has otherwise."); - } else if (regionNode.isInState(State.OPENING)) { - try { - if (!reportTransition(regionNode, serverNode, TransitionCode.OPENED, 0)) { - LOG.warn(regionNode.toString() + " reported OPEN on server=" + serverName + - " but state has otherwise AND NO procedure is running"); - } - } catch (UnexpectedStateException e) { - LOG.warn(regionNode.toString() + " reported unexpteced OPEN: " + e.getMessage(), e); - } - } - } else if (!regionNode.isInState(State.CLOSING, State.SPLITTING)) { - long diff = regionNode.getLastUpdate() - EnvironmentEdgeManager.currentTime(); - if (diff > 1000/*One Second... make configurable if an issue*/) { - // So, we can get report that a region is CLOSED or SPLIT because a heartbeat - // came in at about same time as a region transition. Make sure there is some - // elapsed time between killing remote server. - throw new UnexpectedStateException(regionNode.toString() + - " reported an unexpected OPEN; time since last update=" + diff); - } + RegionStateNode regionNode = regionStates.getRegionStateNodeFromName(regionName); + if (regionNode == null) { + LOG.warn("No region state node for {}, it should already be on {}", + Bytes.toStringBinary(regionName), serverName); + } + regionNode.lock(); + try { + long diff = EnvironmentEdgeManager.currentTime() - regionNode.getLastUpdate(); + if (regionNode.isInState(State.OPENING, State.OPEN)) { + // This is possible as a region server has just closed a region but the region server + // report is generated before the closing, but arrive after the closing. Make sure there + // is some elapsed time so less false alarms. + if (!regionNode.getRegionLocation().equals(serverName) && diff > 1000) { + LOG.warn("{} reported OPEN on server={} but state has otherwise", regionNode, + serverName); + } + } else if (!regionNode.isInState(State.CLOSING, State.SPLITTING)) { + // So, we can get report that a region is CLOSED or SPLIT because a heartbeat + // came in at about same time as a region transition. Make sure there is some + // elapsed time so less false alarms. + if (diff > 1000) { + LOG.warn("{} reported an unexpected OPEN on {}; time since last update={}ms", + regionNode, serverName, diff); } - } finally { - regionNode.unlock(); } + } finally { + regionNode.unlock(); } - } catch (IOException e) { - //See HBASE-21421, we can count on reportRegionStateTransition calls - //We only log a warming here. It could be a network lag. - LOG.warn("Failed to checkOnlineRegionsReport, maybe due to network lag, " - + "if this message continues, be careful of double assign", e); } } @@ -1905,10 +1862,6 @@ public class AssignmentManager implements ServerListener { wakeServerReportEvent(serverNode); } - private void killRegionServer(final ServerStateNode serverNode) { - master.getServerManager().expireServer(serverNode.getServerName()); - } - @VisibleForTesting MasterServices getMaster() { return master; diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/MockMasterServices.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/MockMasterServices.java index c0dc72c39a..5a1f87d6f3 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/MockMasterServices.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/MockMasterServices.java @@ -33,7 +33,6 @@ import org.apache.hadoop.hbase.ServerMetricsBuilder; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableDescriptors; import org.apache.hadoop.hbase.TableName; -import org.apache.hadoop.hbase.YouAreDeadException; import org.apache.hadoop.hbase.client.ClusterConnection; import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.client.HConnectionTestingUtility; @@ -78,7 +77,6 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos.RegionActi import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos.RegionActionResult; import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos.ResultOrException; - /** * A mocked master services. * Tries to fake it. May not always work. @@ -126,13 +124,9 @@ public class MockMasterServices extends MockNoopMasterServices { @Override protected boolean waitServerReportEvent(ServerName serverName, Procedure proc) { // Make a report with current state of the server 'serverName' before we call wait.. - SortedSet regions = regionsToRegionServers.get(serverName); - try { - getAssignmentManager().reportOnlineRegions(serverName, - regions == null? new HashSet(): regions); - } catch (YouAreDeadException e) { - throw new RuntimeException(e); - } + SortedSet regions = regionsToRegionServers.get(serverName); + getAssignmentManager().reportOnlineRegions(serverName, + regions == null ? new HashSet() : regions); return super.waitServerReportEvent(serverName, proc); } }; diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestReportOnlineRegionsRace.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestReportOnlineRegionsRace.java new file mode 100644 index 0000000000..371897bdbc --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestReportOnlineRegionsRace.java @@ -0,0 +1,188 @@ +/** + * 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 static org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.RegionStateTransitionState.REGION_STATE_TRANSITION_CONFIRM_OPENED_VALUE; +import static org.junit.Assert.assertEquals; + +import java.io.IOException; +import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Future; +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.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.Table; +import org.apache.hadoop.hbase.master.HMaster; +import org.apache.hadoop.hbase.master.MasterServices; +import org.apache.hadoop.hbase.master.RegionPlan; +import org.apache.hadoop.hbase.master.RegionState; +import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv; +import org.apache.hadoop.hbase.procedure2.ProcedureExecutor; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.IdLock; +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.ReportRegionStateTransitionRequest; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.ReportRegionStateTransitionResponse; + +@Category({ MasterTests.class, MediumTests.class }) +public class TestReportOnlineRegionsRace { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestReportOnlineRegionsRace.class); + + private static volatile CountDownLatch ARRIVE_RS_REPORT; + private static volatile CountDownLatch RESUME_RS_REPORT; + private static volatile CountDownLatch FINISH_RS_REPORT; + + private static volatile CountDownLatch RESUME_REPORT_STATE; + + private static final class AssignmentManagerForTest extends AssignmentManager { + + public AssignmentManagerForTest(MasterServices master) { + super(master); + } + + @Override + public void reportOnlineRegions(ServerName serverName, Set regionNames) { + if (ARRIVE_RS_REPORT != null) { + ARRIVE_RS_REPORT.countDown(); + try { + RESUME_RS_REPORT.await(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } + super.reportOnlineRegions(serverName, regionNames); + if (FINISH_RS_REPORT != null) { + FINISH_RS_REPORT.countDown(); + } + } + + @Override + public ReportRegionStateTransitionResponse reportRegionStateTransition( + ReportRegionStateTransitionRequest req) throws PleaseHoldException { + if (RESUME_REPORT_STATE != null) { + try { + RESUME_REPORT_STATE.await(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } + return super.reportRegionStateTransition(req); + } + + } + + public static final class HMasterForTest extends HMaster { + + public HMasterForTest(Configuration conf) throws IOException, KeeperException { + super(conf); + } + + @Override + protected AssignmentManager createAssignmentManager(MasterServices master) { + return new AssignmentManagerForTest(master); + } + } + + private static final HBaseTestingUtility UTIL = new HBaseTestingUtility(); + + private static TableName NAME = TableName.valueOf("Race"); + + private static byte[] CF = Bytes.toBytes("cf"); + + @BeforeClass + public static void setUp() throws Exception { + UTIL.getConfiguration().setClass(HConstants.MASTER_IMPL, HMasterForTest.class, HMaster.class); + UTIL.getConfiguration().setInt("hbase.regionserver.msginterval", 1000); + UTIL.startMiniCluster(1); + UTIL.createTable(NAME, CF); + UTIL.waitTableAvailable(NAME); + } + + @AfterClass + public static void tearDown() throws Exception { + UTIL.shutdownMiniCluster(); + } + + @Test + public void testRace() throws Exception { + RegionInfo region = UTIL.getMiniHBaseCluster().getRegions(NAME).get(0).getRegionInfo(); + ProcedureExecutor procExec = + UTIL.getMiniHBaseCluster().getMaster().getMasterProcedureExecutor(); + AssignmentManager am = UTIL.getMiniHBaseCluster().getMaster().getAssignmentManager(); + RegionStateNode rsn = am.getRegionStates().getRegionStateNode(region); + + // halt a regionServerReport + RESUME_RS_REPORT = new CountDownLatch(1); + ARRIVE_RS_REPORT = new CountDownLatch(1); + FINISH_RS_REPORT = new CountDownLatch(1); + + ARRIVE_RS_REPORT.await(); + + // schedule a TRSP to REOPEN the region + RESUME_REPORT_STATE = new CountDownLatch(1); + Future future = + am.moveAsync(new RegionPlan(region, rsn.getRegionLocation(), rsn.getRegionLocation())); + TransitRegionStateProcedure proc = + procExec.getProcedures().stream().filter(p -> p instanceof TransitRegionStateProcedure) + .filter(p -> !p.isFinished()).map(p -> (TransitRegionStateProcedure) p).findAny().get(); + IdLock procExecLock = procExec.getProcExecutionLock(); + // a CloseRegionProcedure and then the OpenRegionProcedure we want to block + IdLock.Entry lockEntry = procExecLock.getLockEntry(proc.getProcId() + 2); + // resume the reportRegionStateTransition to finish the CloseRegionProcedure + RESUME_REPORT_STATE.countDown(); + // wait until we schedule the OpenRegionProcedure + UTIL.waitFor(10000, + () -> proc.getCurrentStateId() == REGION_STATE_TRANSITION_CONFIRM_OPENED_VALUE); + // the region should be in OPENING state + assertEquals(RegionState.State.OPENING, rsn.getState()); + // resume the region server report + RESUME_RS_REPORT.countDown(); + // wait until it finishes, it will find that the region is opened on the rs + FINISH_RS_REPORT.await(); + // let the OpenRegionProcedure go + procExecLock.releaseLockEntry(lockEntry); + // wait until the TRSP is done + future.get(); + + // confirm that the region can still be write, i.e, the regionServerReport method should not + // change the region state to OPEN + try (Table table = UTIL.getConnection().getTableBuilder(NAME, null).setWriteRpcTimeout(1000) + .setOperationTimeout(2000).build()) { + table.put( + new Put(Bytes.toBytes("key")).addColumn(CF, Bytes.toBytes("cq"), Bytes.toBytes("val"))); + } + } +} -- 2.17.1