HBASE-30169 change the place to check the isSplitOrMerged

Author: Umeshkumar9414

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

@@ -1892,9 +1892,10 @@ public void visitRegionState(Result result, final RegionInfo regionInfo, final S
       }
       // add regions to RIT while visiting the meta
       regionInTransitionTracker.handleRegionStateNodeOperation(regionNode);
-      // If region location of region belongs to a dead server mark the region crashed
+      // If region is supposed to be serve traffic (NOT split and merged) and location of region
+      // belongs to a dead server mark the region crashed
       if (
-        regionNode.getRegionLocation() != null
+        regionNode.getRegionLocation() != null && !AssignmentManagerUtil.isSplitOrMerged(regionNode)
           && master.getServerManager().isServerDead(regionNode.getRegionLocation())
       ) {
         long timeOfCrash = master.getServerManager().getDeadServers()

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

@@ -35,6 +35,7 @@
 import org.apache.hadoop.hbase.client.RegionInfo;
 import org.apache.hadoop.hbase.client.RegionReplicaUtil;
 import org.apache.hadoop.hbase.favored.FavoredNodesManager;
+import org.apache.hadoop.hbase.master.RegionState;
 import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
 import org.apache.hadoop.hbase.util.FutureUtils;
 import org.apache.hadoop.hbase.wal.WALSplitUtil;
@@ -298,4 +299,16 @@ static void checkClosedRegion(MasterProcedureEnv env, RegionInfo regionInfo) thr
         + ", abort split/merge to prevent data loss");
     }
   }
+
+  /**
+   * For splitting, need to test both region info and state, and will return true if either of the
+   * test returns true. Please see the comments in
+   * {@link AssignmentManager#markRegionAsSplit(RegionInfo, ServerName, RegionInfo, RegionInfo)} for
+   * more details on why we need to test two conditions.
+   */
+  static boolean isSplitOrMerged(RegionStateNode regionStateNode) {
+    return regionStateNode.getState() == RegionState.State.SPLIT
+      || regionStateNode.getRegionInfo().isSplit()
+      || regionStateNode.getState() == RegionState.State.MERGED;
+  }
 }

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

@@ -41,8 +41,10 @@ public class RegionInTransitionTracker {
 
   private final List<RegionState.State> ENABLE_TABLE_REGION_STATE = List.of(RegionState.State.OPEN);
 
-  // DO NOT USE containsKey() on regionInTransition map as MutableRegionInfo.COMPARATOR considers
-  // offline/split flags.
+  // DO NOT USE containsKey()/remove() on regionInTransition with a different RegionInfo instance:
+  // this map is ordered by RegionInfo.COMPARATOR, and that comparator includes the offline flag.
+  // Lookups can therefore fail if the RegionInfo used as the key has a different offline value,
+  // even when it refers to the same region. Offline value changes with splitting.
   private final ConcurrentSkipListMap<RegionInfo, RegionStateNode> regionInTransition =
     new ConcurrentSkipListMap<>(RegionInfo.COMPARATOR);
 
@@ -54,7 +56,7 @@ public class RegionInTransitionTracker {
    * other servers.
    */
   public void regionCrashed(RegionStateNode regionStateNode) {
-    if (isReplica(regionStateNode) || isSplitOrMerged(regionStateNode)) {
+    if (isReplica(regionStateNode)) {
       return;
     }
 
@@ -84,7 +86,7 @@ public void handleRegionStateNodeOperation(RegionStateNode regionStateNode) {
       tableEnabled ? ENABLE_TABLE_REGION_STATE : DISABLE_TABLE_REGION_STATE;
 
     // if region is merged or split it should not be in RIT list
-    if (isSplitOrMerged(regionStateNode)) {
+    if (AssignmentManagerUtil.isSplitOrMerged(regionStateNode)) {
       if (removeRegionInTransition(regionStateNode.getRegionInfo())) {
         LOG.debug("Removed {} from RIT list as it is split or merged",
           regionStateNode.getRegionInfo().getEncodedName());
@@ -102,12 +104,6 @@ public void handleRegionStateNodeOperation(RegionStateNode regionStateNode) {
     }
   }
 
-  private static boolean isSplitOrMerged(RegionStateNode regionStateNode) {
-    return regionStateNode.getState() == RegionState.State.SPLIT
-      || regionStateNode.getState() == RegionState.State.MERGED
-      || regionStateNode.getRegionInfo().isSplit();
-  }
-
   private boolean isTableEnabled(TableName tableName) {
     if (tableStateManager != null) {
       return tableStateManager.isTableState(tableName, TableState.State.ENABLED,

hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionSplit.java

@@ -21,6 +21,7 @@
 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.assertTrue;
 
 import java.util.List;
 import java.util.Map;
@@ -168,8 +169,8 @@ public void testRITWithSplitTableRegion() throws Exception {
 
     assertNotNull(regions, "not able to find a splittable region");
     assertEquals(1, regions.length, "not able to find a splittable region");
-    assertEquals(0,
-      UTIL.getHBaseCluster().getMaster().getAssignmentManager().getRegionsInTransitionCount());
+    assertFalse(AssignmentTestingUtil.isRegionInTransition(regions[0],
+      UTIL.getHBaseCluster().getMaster().getAssignmentManager()));
 
     ServerName targetRS = UTIL.getHBaseCluster().getMaster().getAssignmentManager()
       .getRegionStates().getRegionServerOfRegion(regions[0]);
@@ -181,22 +182,25 @@ public void testRITWithSplitTableRegion() throws Exception {
     ProcedureTestingUtility.assertProcNotFailed(procExec, procId);
 
     assertEquals(2, UTIL.getHBaseCluster().getRegions(tableName).size(), "not able to split table");
-    assertEquals(0,
-      UTIL.getHBaseCluster().getMaster().getAssignmentManager().getRegionsInTransitionCount());
+    assertFalse(AssignmentTestingUtil.isRegionInTransition(regions[0],
+      UTIL.getHBaseCluster().getMaster().getAssignmentManager()));
     // As there are only 3 RS, start one more RS before expiring one
     UTIL.getHBaseCluster().startRegionServer();
 
-    // stop RS holding split parent
+    // We don't want SCP to complete so kill PR it after store update
+    ProcedureTestingUtility
+      .toggleKillAfterStoreUpdate(UTIL.getHBaseCluster().getMaster().getMasterProcedureExecutor());
+    // stop RS holding split parent to create SCP and add RS into deadServerList
     UTIL.getHBaseCluster().getMaster().getServerManager().expireServer(targetRS);
 
     // stop master
     UTIL.getHBaseCluster().stopMaster(0);
     UTIL.getHBaseCluster().waitOnMaster(0);
-    Thread.sleep(500);
 
     // restart master
-    JVMClusterUtil.MasterThread t = UTIL.getHBaseCluster().startMaster();
-    UTIL.getHBaseCluster().waitForActiveAndReadyMaster(10000);
+    UTIL.getHBaseCluster().startMaster();
+    assertTrue(UTIL.getHBaseCluster().waitForActiveAndReadyMaster(30000),
+      "Master failed to initialize in in 30 seconds");
     UTIL.invalidateConnection();
 
     assertFalse(AssignmentTestingUtil.isRegionInTransition(regions[0],