@HBASE-28158 code review changes

ukumawat · ukumawat · commit 2da766ebc90b · 2025-11-12T01:27:47.000+05:30
diff --git a/hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon b/hbase-server/src/main/jamon/org/apache/hadoop/hbase/tmpl/master/MasterStatusTmpl.jamon
@@ -724,7 +724,7 @@ AssignmentManager assignmentManager = master.getAssignmentManager();
     <tr>
     	<th></th>
         <td><% deadServerName %></td>
-        <td><% deadServerUtil.getTimeOfDeath(deadServerName) %></td>
+        <td><% new Date(deadServerUtil.getTimeOfDeath(deadServerName)) %></td>
         <%if rsGroupName != null %>
             <td><% rsGroupName %></td>
         </%if>
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DeadServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/DeadServer.java
@@ -19,7 +19,6 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -75,7 +74,7 @@ synchronized void putIfAbsent(ServerName sn) {
     this.deadServers.putIfAbsent(sn, EnvironmentEdgeManager.currentTime());
   }
 
-  synchronized void putIfAbsent(ServerName sn, Long crashedTime) {
+  synchronized void putIfAbsent(ServerName sn, long crashedTime) {
     this.deadServers.putIfAbsent(sn, crashedTime);
   }
 
@@ -168,9 +167,9 @@ synchronized List<Pair<ServerName, Long>> copyDeadServersSince(long ts) {
    * @param deadServerName the dead server name
    * @return the date when the server died
    */
-  public synchronized Date getTimeOfDeath(final ServerName deadServerName) {
+  public synchronized long getTimeOfDeath(final ServerName deadServerName) {
     Long time = deadServers.get(deadServerName);
-    return time == null ? null : new Date(time);
+    return time == null ? 0 : time;
   }
 
   /**
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
@@ -2116,7 +2116,8 @@ public BalanceResponse balance(BalanceRequest request) throws IOException {
         if (!request.isIgnoreRegionsInTransition() || metaInTransition) {
           LOG.info("Not running balancer (ignoreRIT=false" + ", metaRIT=" + metaInTransition
             + ") because " + assignmentManager.getRegionTransitScheduledCount()
-            + " region(s) in transition: " + toPrint + (truncated ? "(truncated list)" : ""));
+            + " region(s) are scheduled to transit " + toPrint
+            + (truncated ? "(truncated list)" : ""));
           return responseBuilder.build();
         }
       }
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
@@ -23,7 +23,6 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -391,7 +390,7 @@ public void setupRIT(List<TransitRegionStateProcedure> procs) {
           return;
         }
       }
-      LOG.info("Attach {} to {} to restore", proc, regionNode);
+      LOG.info("Attach {} to {}", proc, regionNode);
       regionNode.setProcedure(proc);
     });
   }
@@ -1888,10 +1887,10 @@ public void visitRegionState(Result result, final RegionInfo regionInfo, final S
         regionNode.getRegionLocation() != null
           && master.getServerManager().isServerDead(regionNode.getRegionLocation())
       ) {
-        Date timeOfCrash =
+        long timeOfCrash =
           master.getServerManager().getDeadServers().getTimeOfDeath(regionNode.getRegionLocation());
-        if (timeOfCrash != null) {
-          regionNode.crashed(timeOfCrash.getTime());
+        if (timeOfCrash != 0) {
+          regionNode.crashed(timeOfCrash);
         }
         regionInTransitionTracker.regionCrashed(regionNode);
       }
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionInTransitionTracker.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionInTransitionTracker.java
@@ -28,6 +28,9 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+/**
+ * Tracks regions that are currently in transition (RIT) - those not yet in their terminal state.
+ */
 @InterfaceAudience.Private
 public class RegionInTransitionTracker {
   private static final Logger LOG = LoggerFactory.getLogger(RegionInTransitionTracker.class);
@@ -50,6 +53,11 @@ public boolean isRegionInTransition(final RegionInfo regionInfo) {
     return regionInTransition.containsKey(regionInfo);
   }
 
+  /**
+   * Handles a region whose hosting RegionServer has crashed. When a RegionServer fails, all regions
+   * it was hosting are automatically added to the RIT list since they need to be reassigned to
+   * other servers.
+   */
   public void regionCrashed(RegionStateNode regionStateNode) {
     if (regionStateNode.getRegionInfo().getReplicaId() != RegionInfo.DEFAULT_REPLICA_ID) {
       return;
@@ -61,6 +69,14 @@ public void regionCrashed(RegionStateNode regionStateNode) {
     }
   }
 
+  /**
+   * Processes a region state change and updates the RIT tracking accordingly. This is the core
+   * method that determines whether a region should be added to or removed from the RIT list based
+   * on its current state and the table's enabled/disabled status. This method should be called
+   * whenever a region state changes get stored to hbase:meta Note: Only default replicas (replica
+   * ID 0) are tracked. Read replicas are ignored.
+   * @param regionStateNode the region state node with the current state information
+   */
   public void handleRegionStateNodeOperation(RegionStateNode regionStateNode) {
     // only consider default replica for availability
     if (regionStateNode.getRegionInfo().getReplicaId() != RegionInfo.DEFAULT_REPLICA_ID) {
@@ -95,6 +111,13 @@ public void handleRegionStateNodeOperation(RegionStateNode regionStateNode) {
     }
   }
 
+  /**
+   * Handles the deletion of a region by removing it from RIT tracking. This is called when a region
+   * is permanently removed from the cluster, typically after a successful merge operation where the
+   * parent regions are cleaned up. During table deletion, table should be already disabled and all
+   * the region are already OFFLINE
+   * @param regionInfo the region being deleted
+   */
   public void handleRegionDelete(RegionInfo regionInfo) {
     removeRegionInTransition(regionInfo);
   }
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateNode.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateNode.java
@@ -68,7 +68,7 @@
 public class RegionStateNode implements Comparable<RegionStateNode> {
 
   private static final Logger LOG = LoggerFactory.getLogger(RegionStateNode.class);
-  private final AtomicInteger trspCounter;
+  private final AtomicInteger activeTransitProcedureCount;
 
   private static final class AssignmentProcedureEvent extends ProcedureEvent<RegionInfo> {
     public AssignmentProcedureEvent(final RegionInfo regionInfo) {
@@ -102,11 +102,11 @@ public AssignmentProcedureEvent(final RegionInfo regionInfo) {
 
   private volatile long openSeqNum = HConstants.NO_SEQNUM;
 
-  RegionStateNode(RegionInfo regionInfo, AtomicInteger trspCounter) {
+  RegionStateNode(RegionInfo regionInfo, AtomicInteger activeTransitProcedureCount) {
     this.regionInfo = regionInfo;
     this.event = new AssignmentProcedureEvent(regionInfo);
     this.lock = new RegionStateNodeLock(regionInfo);
-    this.trspCounter = trspCounter;
+    this.activeTransitProcedureCount = activeTransitProcedureCount;
   }
 
   /**
@@ -211,13 +211,13 @@ public ServerName setRegionLocation(final ServerName serverName) {
   public TransitRegionStateProcedure setProcedure(TransitRegionStateProcedure proc) {
     assert this.procedure == null;
     this.procedure = proc;
-    trspCounter.incrementAndGet();
+    activeTransitProcedureCount.incrementAndGet();
     return proc;
   }
 
   public void unsetProcedure(TransitRegionStateProcedure proc) {
     assert this.procedure == proc;
-    trspCounter.decrementAndGet();
+    activeTransitProcedureCount.decrementAndGet();
     this.procedure = null;
   }
 
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java
@@ -53,7 +53,7 @@ public class RegionStates {
 
   private final Object regionsMapLock = new Object();
 
-  private final AtomicInteger trspCounter = new AtomicInteger(0);
+  private final AtomicInteger activeTransitProcedureCount = new AtomicInteger(0);
 
   // TODO: Replace the ConcurrentSkipListMaps
   /**
@@ -105,7 +105,7 @@ public boolean isRegionInRegionStates(final RegionInfo hri) {
   RegionStateNode createRegionStateNode(RegionInfo regionInfo) {
     synchronized (regionsMapLock) {
       RegionStateNode node = regionsMap.computeIfAbsent(regionInfo.getRegionName(),
-        key -> new RegionStateNode(regionInfo, trspCounter));
+        key -> new RegionStateNode(regionInfo, activeTransitProcedureCount));
 
       if (encodedRegionsMap.get(regionInfo.getEncodedName()) != node) {
         encodedRegionsMap.put(regionInfo.getEncodedName(), node);
@@ -602,7 +602,7 @@ public void addToOfflineRegions(final RegionStateNode regionNode) {
   }
 
   public int getRegionTransitScheduledCount() {
-    return trspCounter.get();
+    return activeTransitProcedureCount.get();
   }
 
   // ==========================================================================
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ProcedureSyncWait.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ProcedureSyncWait.java
@@ -31,8 +31,8 @@
 import org.apache.hadoop.hbase.client.RegionInfoBuilder;
 import org.apache.hadoop.hbase.exceptions.TimeoutIOException;
 import org.apache.hadoop.hbase.master.RegionState;
-import org.apache.hadoop.hbase.master.assignment.AssignmentManager;
 import org.apache.hadoop.hbase.master.assignment.RegionStateNode;
+import org.apache.hadoop.hbase.master.assignment.RegionStates;
 import org.apache.hadoop.hbase.procedure2.Procedure;
 import org.apache.hadoop.hbase.procedure2.ProcedureExecutor;
 import org.apache.hadoop.hbase.quotas.MasterQuotaManager;
@@ -246,14 +246,13 @@ protected static void waitMetaRegions(final MasterProcedureEnv env) throws IOExc
 
   protected static void waitRegionInTransition(final MasterProcedureEnv env,
     final List<RegionInfo> regions) throws IOException {
-    final AssignmentManager assignmentManager = env.getAssignmentManager();
+    final RegionStates states = env.getAssignmentManager().getRegionStates();
     for (final RegionInfo region : regions) {
       ProcedureSyncWait.waitFor(env, "regions " + region.getRegionNameAsString() + " in transition",
         new ProcedureSyncWait.Predicate<Boolean>() {
           @Override
           public Boolean evaluate() throws IOException {
-            return !assignmentManager.getRegionStates().getRegionStateNode(region)
-              .isTransitionScheduled();
+            return !states.getRegionStateNode(region).isTransitionScheduled();
           }
         });
     }
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ServerCrashProcedure.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ServerCrashProcedure.java
@@ -169,6 +169,8 @@ protected Flow executeFromState(MasterProcedureEnv env, ServerCrashState state)
           LOG.info("Start " + this);
           // If carrying meta, process it first. Else, get list of regions on crashed server.
           if (this.carryingMeta) {
+            env.getAssignmentManager()
+              .markRegionsAsCrashed(List.of(RegionInfoBuilder.FIRST_META_REGIONINFO), this);
             setNextState(ServerCrashState.SERVER_CRASH_SPLIT_META_LOGS);
           } else {
             setNextState(ServerCrashState.SERVER_CRASH_GET_REGIONS);
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDeadServer.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestDeadServer.java
@@ -18,6 +18,7 @@
 package org.apache.hadoop.hbase.master;
 
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 
 import java.util.List;
@@ -83,7 +84,7 @@ public void testIsDead() {
     assertTrue(ds.isDeadServer(deadServer));
     Set<ServerName> deadServerNames = ds.copyServerNames();
     for (ServerName eachDeadServer : deadServerNames) {
-      Assert.assertNotNull(ds.getTimeOfDeath(eachDeadServer));
+      assertNotEquals(0, ds.getTimeOfDeath(eachDeadServer));
     }
     final ServerName deadServerHostComingAlive = ServerName.valueOf("127.0.0.1", 9090, 223341L);
     assertTrue(ds.cleanPreviousInstance(deadServerHostComingAlive));

Original file line number	Diff line number	Diff line change
`@@ -2116,7 +2116,8 @@ public BalanceResponse balance(BalanceRequest request) throws IOException {`
`2116`	`2116`	`if (!request.isIgnoreRegionsInTransition() \|\| metaInTransition) {`
`2117`	`2117`	`LOG.info("Not running balancer (ignoreRIT=false" + ", metaRIT=" + metaInTransition`
`2118`	`2118`	`+ ") because " + assignmentManager.getRegionTransitScheduledCount()`
`2119`		`- + " region(s) in transition: " + toPrint + (truncated ? "(truncated list)" : ""));`
	`2119`	`+ + " region(s) are scheduled to transit " + toPrint`
	`2120`	`+ + (truncated ? "(truncated list)" : ""));`
`2120`	`2121`	`return responseBuilder.build();`
`2121`	`2122`	`}`
`2122`	`2123`	`}`
Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,6 @@`
`23`	`23`	`import java.util.Collection;`
`24`	`24`	`import java.util.Collections;`
`25`	`25`	`import java.util.Comparator;`
`26`		`-import java.util.Date;`
`27`	`26`	`import java.util.HashMap;`
`28`	`27`	`import java.util.HashSet;`
`29`	`28`	`import java.util.List;`
`@@ -391,7 +390,7 @@ public void setupRIT(List<TransitRegionStateProcedure> procs) {`
`391`	`390`	`return;`
`392`	`391`	`}`
`393`	`392`	`}`
`394`		`- LOG.info("Attach {} to {} to restore", proc, regionNode);`
	`393`	`+ LOG.info("Attach {} to {}", proc, regionNode);`
`395`	`394`	`regionNode.setProcedure(proc);`
`396`	`395`	`});`
`397`	`396`	`}`
`@@ -1888,10 +1887,10 @@ public void visitRegionState(Result result, final RegionInfo regionInfo, final S`
`1888`	`1887`	`regionNode.getRegionLocation() != null`
`1889`	`1888`	`&& master.getServerManager().isServerDead(regionNode.getRegionLocation())`
`1890`	`1889`	`) {`
`1891`		`- Date timeOfCrash =`
	`1890`	`+ long timeOfCrash =`
`1892`	`1891`	`master.getServerManager().getDeadServers().getTimeOfDeath(regionNode.getRegionLocation());`
`1893`		`- if (timeOfCrash != null) {`
`1894`		`- regionNode.crashed(timeOfCrash.getTime());`
	`1892`	`+ if (timeOfCrash != 0) {`
	`1893`	`+ regionNode.crashed(timeOfCrash);`
`1895`	`1894`	`}`
`1896`	`1895`	`regionInTransitionTracker.regionCrashed(regionNode);`
`1897`	`1896`	`}`