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 5baf30846e08..684816106623 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 @@ -695,9 +695,6 @@ public boolean isMetaLoaded() { *

*/ public void checkIfShouldMoveSystemRegionAsync() { - // TODO: Fix this thread. If a server is killed and a new one started, this thread thinks that - // it should 'move' the system tables from the old server to the new server but - // ServerCrashProcedure is on it; and it will take care of the assign without dataloss. if (this.master.getServerManager().countOfRegionServers() <= 1) { return; } @@ -711,37 +708,37 @@ public void checkIfShouldMoveSystemRegionAsync() { try { synchronized (checkIfShouldMoveSystemRegionLock) { List plans = new ArrayList<>(); - // TODO: I don't think this code does a good job if all servers in cluster have same - // version. It looks like it will schedule unnecessary moves. for (ServerName server : getExcludedServersForSystemTable()) { - if (master.getServerManager().isServerDead(server)) { - // TODO: See HBASE-18494 and HBASE-18495. Though getExcludedServersForSystemTable() - // considers only online servers, the server could be queued for dead server - // processing. As region assignments for crashed server is handled by - // ServerCrashProcedure, do NOT handle them here. The goal is to handle this through - // regular flow of LoadBalancer as a favored node and not to have this special - // handling. + if (!master.getServerManager().isServerOnline(server)) { + // Leave regions on crashed servers to ServerCrashProcedure. continue; } - List regionsShouldMove = getSystemTables(server); - if (!regionsShouldMove.isEmpty()) { - for (RegionInfo regionInfo : regionsShouldMove) { - // null value for dest forces destination server to be selected by balancer - RegionPlan plan = new RegionPlan(regionInfo, server, null); - if (regionInfo.isMetaRegion()) { - // Must move meta region first. - LOG.info("Async MOVE of {} to newer Server={}", regionInfo.getEncodedName(), - server); - moveAsync(plan); - } else { - plans.add(plan); - } - } + for (RegionInfo regionInfo : getSystemTables(server)) { + // null value for dest forces destination server to be selected by balancer + plans.add(new RegionPlan(regionInfo, server, null)); + } + } + // Submit meta moves before other system regions, and submit each plan only once. + plans.sort(Comparator.comparing(plan -> !plan.getRegionInfo().isMetaRegion())); + for (RegionPlan plan : plans) { + RegionStateNode regionNode = regionStates.getRegionStateNode(plan.getRegionInfo()); + if (regionNode == null || !master.getServerManager().isServerOnline(plan.getSource())) { + continue; } - for (RegionPlan plan : plans) { - LOG.info("Async MOVE of {} to newer Server={}", plan.getRegionInfo().getEncodedName(), - server); - moveAsync(plan); + if (regionNode.isTransitionScheduled()) { + LOG.debug("Skip system region move for {}; a transition is already scheduled", + regionNode); + continue; + } + try { + // balance checks the source location; preTransitCheck still guards against a + // transition starting after the check above. + if (balance(plan) != null) { + LOG.info("Async MOVE of {} from {} to a newer RegionServer", + plan.getRegionInfo().getEncodedName(), plan.getSource()); + } + } catch (HBaseIOException e) { + LOG.warn("Failed system region move {}, skipping this plan", plan, e); } } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMoveSystemRegions.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMoveSystemRegions.java new file mode 100644 index 000000000000..cebd0118eb80 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMoveSystemRegions.java @@ -0,0 +1,175 @@ +/* + * 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.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doCallRealMethod; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseIOException; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.client.RegionInfoBuilder; +import org.apache.hadoop.hbase.master.MasterServices; +import org.apache.hadoop.hbase.master.RegionPlan; +import org.apache.hadoop.hbase.master.RegionState.State; +import org.apache.hadoop.hbase.master.ServerManager; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +@Tag(MasterTests.TAG) +@Tag(SmallTests.TAG) +public class TestMoveSystemRegions { + + private static final ServerName OLD_SERVER = ServerName.valueOf("old", 16020, 1); + private static final ServerName OTHER_OLD_SERVER = ServerName.valueOf("other-old", 16020, 1); + private static final ServerName NEW_SERVER = ServerName.valueOf("new", 16020, 1); + private static final RegionInfo META = RegionInfoBuilder.FIRST_META_REGIONINFO; + private static final RegionInfo SYSTEM_REGION = + RegionInfoBuilder.newBuilder(TableName.valueOf("hbase:test")).build(); + + private AssignmentManager am; + private ServerManager serverManager; + private final List movedRegions = new ArrayList<>(); + + @BeforeEach + public void setUp() throws Exception { + MasterServices master = mock(MasterServices.class); + when(master.getConfiguration()).thenReturn(new Configuration(false)); + serverManager = mock(ServerManager.class); + when(master.getServerManager()).thenReturn(serverManager); + when(serverManager.countOfRegionServers()).thenReturn(3); + when(serverManager.getOnlineServersList()) + .thenReturn(List.of(OLD_SERVER, OTHER_OLD_SERVER, NEW_SERVER)); + when(serverManager.isServerOnline(any())).thenReturn(true); + when(master.getRegionServerVersion(OLD_SERVER)).thenReturn("2.6.4"); + when(master.getRegionServerVersion(OTHER_OLD_SERVER)).thenReturn("2.6.4"); + when(master.getRegionServerVersion(NEW_SERVER)).thenReturn("4.0.0"); + am = spy(new AssignmentManager(master, null)); + doAnswer(invocation -> { + RegionPlan plan = invocation.getArgument(0); + movedRegions.add(plan.getRegionInfo()); + return CompletableFuture.completedFuture(null); + }).when(am).moveAsync(any()); + } + + private RegionStateNode addRegion(RegionInfo region, ServerName server) { + RegionStateNode node = am.getRegionStates().getOrCreateRegionStateNode(region); + node.setState(State.OPEN); + node.setRegionLocation(server); + am.getRegionStates().createServer(server); + am.getRegionStates().addRegionToServer(node); + return node; + } + + private void checkSystemRegions() throws Exception { + CompletableFuture checker = new CompletableFuture<>(); + doAnswer(invocation -> { + checker.complete(Thread.currentThread()); + return invocation.callRealMethod(); + }).when(am).getExcludedServersForSystemTable(); + am.checkIfShouldMoveSystemRegionAsync(); + Thread thread = checker.get(10, TimeUnit.SECONDS); + thread.join(TimeUnit.SECONDS.toMillis(10)); + assertFalse(thread.isAlive(), "System region check did not finish"); + } + + @Test + public void testSkipMetaInTransition() throws Exception { + RegionStateNode meta = addRegion(META, OLD_SERVER); + meta.setState(State.OPENING); + TransitRegionStateProcedure recovery = mock(TransitRegionStateProcedure.class); + meta.setProcedure(recovery); + addRegion(SYSTEM_REGION, OLD_SERVER); + // Exercise the real preTransitCheck if the compatibility check tries to move meta. + doCallRealMethod().when(am).moveAsync(argThat(p -> p.getRegionInfo().isMetaRegion())); + + checkSystemRegions(); + + assertEquals(List.of(SYSTEM_REGION), movedRegions); + assertSame(recovery, meta.getProcedure()); + assertEquals(State.OPENING, meta.getState()); + verify(am, never()).moveAsync(argThat(p -> p.getRegionInfo().isMetaRegion())); + } + + @Test + public void testSubmitEachPlanOnceAndMetaFirst() throws Exception { + addRegion(SYSTEM_REGION, OLD_SERVER); + addRegion(META, OTHER_OLD_SERVER); + + checkSystemRegions(); + + assertEquals(List.of(META, SYSTEM_REGION), movedRegions); + verify(am, times(2)).moveAsync(any()); + } + + @Test + public void testSkipOfflineSource() throws Exception { + addRegion(META, OLD_SERVER); + addRegion(SYSTEM_REGION, OTHER_OLD_SERVER); + // The online-server snapshot can become stale before we examine its regions. + when(serverManager.isServerOnline(OLD_SERVER)).thenReturn(false); + + checkSystemRegions(); + + assertEquals(List.of(SYSTEM_REGION), movedRegions); + } + + @Test + public void testSkipChangedRegionLocation() throws Exception { + RegionStateNode meta = addRegion(META, OLD_SERVER); + meta.setRegionLocation(NEW_SERVER); + addRegion(SYSTEM_REGION, OTHER_OLD_SERVER); + + checkSystemRegions(); + + assertEquals(List.of(SYSTEM_REGION), movedRegions); + } + + @Test + public void testContinueAfterMoveFailure() throws Exception { + addRegion(META, OLD_SERVER); + addRegion(SYSTEM_REGION, OLD_SERVER); + doThrow(new HBaseIOException("Region entered transition after the check")).when(am) + .moveAsync(argThat(p -> p.getRegionInfo().isMetaRegion())); + + checkSystemRegions(); + + assertEquals(List.of(SYSTEM_REGION), movedRegions); + } +} diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMoveSystemRegionsDuringRecovery.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMoveSystemRegionsDuringRecovery.java new file mode 100644 index 000000000000..8a727ba9c574 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestMoveSystemRegionsDuringRecovery.java @@ -0,0 +1,186 @@ +/* + * 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.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; + +import java.io.IOException; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseTestingUtil; +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.RegionInfo; +import org.apache.hadoop.hbase.client.RegionInfoBuilder; +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.State; +import org.apache.hadoop.hbase.master.procedure.ServerCrashProcedure; +import org.apache.hadoop.hbase.master.region.MasterRegion; +import org.apache.hadoop.hbase.testclassification.LargeTests; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos.RegionStateTransition; +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; + +@Tag(MasterTests.TAG) +@Tag(LargeTests.TAG) +public class TestMoveSystemRegionsDuringRecovery { + + private static final class AssignmentManagerForTest extends AssignmentManager { + private volatile ServerName blockedServer; + private final CountDownLatch metaOpening = new CountDownLatch(1); + private final CountDownLatch resumeReport = new CountDownLatch(1); + + AssignmentManagerForTest(MasterServices master, MasterRegion masterRegion) { + super(master, masterRegion); + } + + @Override + public ReportRegionStateTransitionResponse reportRegionStateTransition( + ReportRegionStateTransitionRequest request) throws PleaseHoldException { + if (ProtobufUtil.toServerName(request.getServer()).equals(blockedServer)) { + for (RegionStateTransition transition : request.getTransitionList()) { + if ( + transition.getTransitionCode() == TransitionCode.OPENED + && ProtobufUtil.toRegionInfo(transition.getRegionInfo(0)).isMetaRegion() + ) { + metaOpening.countDown(); + try { + if (!resumeReport.await(60, TimeUnit.SECONDS)) { + throw new AssertionError("Timed out waiting to resume the meta OPENED report"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new AssertionError(e); + } + } + } + } + return super.reportRegionStateTransition(request); + } + } + + public static final class MasterForTest extends HMaster { + private volatile ServerName newestServer; + + public MasterForTest(Configuration conf) throws IOException { + super(conf); + } + + @Override + protected AssignmentManager createAssignmentManager(MasterServices master, + MasterRegion masterRegion) { + return new AssignmentManagerForTest(master, masterRegion); + } + + @Override + public String getRegionServerVersion(ServerName server) { + return server.equals(newestServer) ? "4.0.0" : "2.6.4"; + } + } + + @Test + public void testMetaRecoveryAfterCompatibilityCheck() throws Exception { + HBaseTestingUtil util = new HBaseTestingUtil(); + util.getConfiguration().setClass(HConstants.MASTER_IMPL, MasterForTest.class, HMaster.class); + AssignmentManagerForTest am = null; + try { + util.startMiniCluster(3); + util.getAdmin().balancerSwitch(false, true); + TableName tableName = TableName.valueOf("testMetaRecoveryAfterCompatibilityCheck"); + util.createTable(tableName, Bytes.toBytes("cf")).close(); + util.waitUntilNoRegionsInTransition(); + MasterForTest master = (MasterForTest) util.getMiniHBaseCluster().getMaster(); + am = (AssignmentManagerForTest) master.getAssignmentManager(); + RegionInfo meta = RegionInfoBuilder.FIRST_META_REGIONINFO; + RegionStateNode node = am.getRegionStates().getRegionStateNode(meta); + ServerName source = node.getRegionLocation(); + ServerName destination = master.getServerManager().getOnlineServersList().stream() + .filter(s -> !s.equals(source)).findFirst().get(); + + // Keep the existing TRSP in OPENING without holding the RegionStateNode lock. + am.blockedServer = source; + Future move = am.moveAsync(new RegionPlan(meta, source, source)); + assertTrue(am.metaOpening.await(30, TimeUnit.SECONDS)); + assertEquals(State.OPENING, node.getState()); + TransitRegionStateProcedure original = node.getProcedure(); + assertNotNull(original); + + // A newer RS becomes available while meta is still opening on the old RS. + master.newestServer = destination; + AssignmentManager checker = spy(am); + CompletableFuture checkThread = new CompletableFuture<>(); + doAnswer(invocation -> { + checkThread.complete(Thread.currentThread()); + return invocation.callRealMethod(); + }).when(checker).getExcludedServersForSystemTable(); + checker.checkIfShouldMoveSystemRegionAsync(); + Thread thread = checkThread.get(10, TimeUnit.SECONDS); + thread.join(TimeUnit.SECONDS.toMillis(10)); + assertFalse(thread.isAlive()); + assertSame(original, node.getProcedure()); + verify(checker, never()).moveAsync(any()); + + util.getMiniHBaseCluster().killRegionServer(source); + util.waitFor(30000, () -> !master.getServerManager().isServerOnline(source)); + am.resumeReport.countDown(); + move.get(60, TimeUnit.SECONDS); + util.waitUntilNoRegionsInTransition(); + util.waitFor(60000, + () -> master.getProcedures().stream().filter(p -> p instanceof ServerCrashProcedure) + .map(p -> (ServerCrashProcedure) p) + .anyMatch(p -> p.getServerName().equals(source) && p.isSuccess())); + assertEquals(State.OPEN, node.getState()); + assertEquals(destination, node.getRegionLocation()); + assertTrue(master.getServerManager().isServerOnline(node.getRegionLocation())); + // A client lookup must read meta successfully after recovery. + try (org.apache.hadoop.hbase.client.RegionLocator locator = + util.getConnection().getRegionLocator(tableName)) { + assertNotNull(locator.getRegionLocation(HConstants.EMPTY_START_ROW, true)); + } + assertEquals(1, util.getAdmin().getRegions(tableName).size()); + } finally { + if (am != null) { + am.resumeReport.countDown(); + } + util.shutdownMiniCluster(); + } + } +}