HBASE-30350 Fix stale Region-In-Transition tracker entries on region deletion

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<RegionInfo> 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.

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java

@@ -284,7 +284,7 @@ static void removeNonDefaultReplicas(MasterProcedureEnv env, Stream<RegionInfo>
     // 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) {

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());

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);

hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java

@@ -506,7 +506,7 @@ private void deleteRegionsFromInMemoryStates(List<RegionInfo> 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<RegionInfo> 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);

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);
   }