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..57483530e49d 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 @@ -1212,7 +1212,7 @@ public void deleteTable(final TableName tableName) throws IOException { regionStateStore.deleteRegions(regions); for (int i = 0; i < regions.size(); ++i) { final RegionInfo regionInfo = regions.get(i); - regionStates.deleteRegion(regionInfo); + deleteRegion(regionInfo); } } @@ -2409,8 +2409,7 @@ public void markRegionAsMerged(final RegionInfo child, final ServerName serverNa RegionInfo[] mergeParents) throws IOException { final RegionStateNode node = regionStates.getOrCreateRegionStateNode(child); for (RegionInfo ri : mergeParents) { - regionStates.deleteRegion(ri); - regionInTransitionTracker.handleRegionDelete(ri); + deleteRegion(ri); } TableDescriptor td = master.getTableDescriptors().get(child.getTable()); @@ -2421,6 +2420,27 @@ public void markRegionAsMerged(final RegionInfo child, final ServerName serverNa } } + /** + * Remove a region that is no longer needed (split/merged parent GC'd, etc) from all in-memory + * assignment tracking. Must remove from both {@link #regionStates} and + * {@link #regionInTransitionTracker} -- they are separate maps and are not kept in sync with each + * other automatically. A caller that only calls {@code regionStates.deleteRegion(...)} can leave + * a stale entry behind in the RIT tracker (visible via the periodic "STUCK Region-In-Transition" + * warning) for as long as the master runs, since nothing else removes it short of a full + * re-derive of state from hbase:meta on master failover. + */ + public void deleteRegion(final RegionInfo regionInfo) { + regionStates.deleteRegion(regionInfo); + regionInTransitionTracker.handleRegionDelete(regionInfo); + } + + /** + * Plural form of {@link #deleteRegion(RegionInfo)}. + */ + public void deleteRegions(final List regionInfos) { + regionInfos.forEach(this::deleteRegion); + } + /* * Favored nodes should be applied only when FavoredNodes balancer is configured and the region * belongs to a non-system table. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java index 8227856c85bf..1d2ebe762fc5 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java @@ -284,7 +284,7 @@ static void removeNonDefaultReplicas(MasterProcedureEnv env, Stream // Remove from in-memory states regions.flatMap(hri -> IntStream.range(1, regionReplication) .mapToObj(i -> RegionReplicaUtil.getRegionInfoForReplica(hri, i))).forEach(hri -> { - env.getAssignmentManager().getRegionStates().deleteRegion(hri); + env.getAssignmentManager().deleteRegion(hri); env.getMasterServices().getServerManager().removeRegion(hri); FavoredNodesManager fnm = env.getMasterServices().getFavoredNodesManager(); if (fnm != null) { diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/GCRegionProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/GCRegionProcedure.java index f1debcd3c3f5..ce241f2b61ea 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/GCRegionProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/GCRegionProcedure.java @@ -111,9 +111,7 @@ protected Flow executeFromState(MasterProcedureEnv env, GCRegionState state) // from CatalogJanitor. AssignmentManager am = masterServices.getAssignmentManager(); if (am != null) { - if (am.getRegionStates() != null) { - am.getRegionStates().deleteRegion(getRegion()); - } + am.deleteRegion(getRegion()); } env.getAssignmentManager().getRegionStateStore().deleteRegion(getRegion()); masterServices.getServerManager().removeRegion(getRegion()); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java index bfa2a52067b7..69a0fba89b1e 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/EnableTableProcedure.java @@ -118,7 +118,7 @@ protected Flow executeFromState(final MasterProcedureEnv env, final EnableTableS for (RegionInfo regionInfo : copyOfRegions) { if (regionInfo.getReplicaId() > (configuredReplicaCount - 1)) { // delete the region from the regionStates - env.getAssignmentManager().getRegionStates().deleteRegion(regionInfo); + env.getAssignmentManager().deleteRegion(regionInfo); // remove it from the list of regions of the table LOG.info("Removed replica={} of {}", regionInfo.getRegionId(), regionInfo); regionsOfTable.remove(regionInfo); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java index bb7a5582f4db..52fcea11152d 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java @@ -506,7 +506,7 @@ private void deleteRegionsFromInMemoryStates(List regionInfos, Maste int regionReplication) { FavoredNodesManager fnm = env.getMasterServices().getFavoredNodesManager(); - env.getAssignmentManager().getRegionStates().deleteRegions(regionInfos); + env.getAssignmentManager().deleteRegions(regionInfos); env.getMasterServices().getServerManager().removeRegions(regionInfos); if (fnm != null) { fnm.deleteFavoredNodesForRegions(regionInfos); @@ -518,7 +518,7 @@ private void deleteRegionsFromInMemoryStates(List regionInfos, Maste for (int i = 1; i < regionReplication; i++) { RegionInfo regionInfoForReplica = RegionReplicaUtil.getRegionInfoForReplica(regionInfo, i); - env.getAssignmentManager().getRegionStates().deleteRegion(regionInfoForReplica); + env.getAssignmentManager().deleteRegion(regionInfoForReplica); env.getMasterServices().getServerManager().removeRegion(regionInfoForReplica); if (fnm != null) { fnm.deleteFavoredNodesForRegion(regionInfoForReplica); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestAssignmentManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestAssignmentManager.java index 01d051779058..25129b231c27 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestAssignmentManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestAssignmentManager.java @@ -18,10 +18,12 @@ 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.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.util.Arrays; import java.util.Collections; import java.util.concurrent.Executors; import java.util.concurrent.Future; @@ -132,6 +134,72 @@ public void testTimeoutThenQueueFull() throws Exception { assertEquals(unassignFailedCount, unassignProcMetrics.getFailedCounter().getCount()); } + @Test + public void testDeleteRegionRemovesStaleRegionInTransitionEntry() throws Exception { + TableName tableName = TableName.valueOf(testMethodName); + RegionInfo hri = createRegionInfo(tableName, 1); + + rsDispatcher.setMockRsExecutor(new GoodRsExecutor()); + waitOnFuture(submitProcedure(createAssignProcedure(hri))); + // This test table is never registered as ENABLED with the TableStateManager, so the tracker + // treats OPEN as a non-terminal state here and keeps the region tracked as in-transition -- + // mirroring a split/merge parent that is left registered in the tracker (in whatever state it + // was last in) but never reaches a state the tracker treats as terminal-and-removable before + // being deleted from regionStates. + assertEquals(State.OPEN, am.getRegionStates().getRegionState(hri).getState()); + assertTrue(isTracked(hri)); + + // Simulate GCRegionProcedure's cleanup of a no-longer-needed region: it must remove the region + // from both regionStates and the RegionInTransitionTracker, not just the former, or the "STUCK + // Region-In-Transition" chore will keep reporting an already-deleted region indefinitely. + am.deleteRegion(hri); + assertNull(am.getRegionStates().getRegionStateNode(hri)); + assertFalse(isTracked(hri)); + } + + @Test + public void testGCRegionProcedureRemovesStaleRegionInTransitionEntry() throws Exception { + TableName tableName = TableName.valueOf(testMethodName); + RegionInfo hri = createRegionInfo(tableName, 1); + + rsDispatcher.setMockRsExecutor(new GoodRsExecutor()); + waitOnFuture(submitProcedure(createAssignProcedure(hri))); + // Same non-terminal-state setup as testDeleteRegionRemovesStaleRegionInTransitionEntry, but + // here we run the actual GCRegionProcedure end-to-end instead of calling + // AssignmentManager#deleteRegion directly, to prove the real procedure used to GC a + // no-longer-needed region (e.g. by CatalogJanitor) also clears the tracker. + assertEquals(State.OPEN, am.getRegionStates().getRegionState(hri).getState()); + assertTrue(isTracked(hri)); + + waitOnFuture(submitProcedure( + new GCRegionProcedure(master.getMasterProcedureExecutor().getEnvironment(), hri))); + assertNull(am.getRegionStates().getRegionStateNode(hri)); + assertFalse(isTracked(hri)); + } + + @Test + public void testDeleteRegionsRemovesStaleRegionInTransitionEntries() throws Exception { + TableName tableName = TableName.valueOf(testMethodName); + RegionInfo hri1 = createRegionInfo(tableName, 1); + RegionInfo hri2 = createRegionInfo(tableName, 2); + + rsDispatcher.setMockRsExecutor(new GoodRsExecutor()); + waitOnFuture(submitProcedure(createAssignProcedure(hri1))); + waitOnFuture(submitProcedure(createAssignProcedure(hri2))); + assertTrue(isTracked(hri1)); + assertTrue(isTracked(hri2)); + + am.deleteRegions(Arrays.asList(hri1, hri2)); + assertNull(am.getRegionStates().getRegionStateNode(hri1)); + assertNull(am.getRegionStates().getRegionStateNode(hri2)); + assertFalse(isTracked(hri1)); + assertFalse(isTracked(hri2)); + } + + private boolean isTracked(RegionInfo hri) { + return am.getRegionsInTransition().stream().anyMatch(node -> node.getRegionInfo().equals(hri)); + } + private void testAssign(final MockRSExecutor executor) throws Exception { testAssign(executor, NREGIONS); }