HBASE-28180 Review the usage of RegionStates.getOrCreateServer

Author: Apache9

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

@@ -135,7 +135,7 @@ public void upgrade(Set<ServerName> deadServersFromPE, Set<ServerName> liveServe
       .forEach(s -> LOG.error("{} has no matching ServerCrashProcedure", s));
     // create ServerNode for all possible live servers from wal directory and master local region
     liveServersBeforeRestart
-      .forEach(sn -> server.getAssignmentManager().getRegionStates().getOrCreateServer(sn));
+      .forEach(sn -> server.getAssignmentManager().getRegionStates().createServer(sn));
     ServerManager serverManager = server.getServerManager();
     synchronized (this) {
       Set<ServerName> liveServers = regionServers;

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

@@ -426,6 +426,7 @@ public ServerName findServerWithSameHostnamePortWithLock(final ServerName server
   void recordNewServerWithLock(final ServerName serverName, final ServerMetrics sl) {
     LOG.info("Registering regionserver=" + serverName);
     this.onlineServers.put(serverName, sl);
+    master.getAssignmentManager().getRegionStates().createServer(serverName);
   }
 
   public ConcurrentNavigableMap<byte[], Long> getFlushedSequenceIdByRegion() {
@@ -603,6 +604,10 @@ synchronized long expireServer(final ServerName serverName, boolean force) {
     }
     LOG.info("Processing expiration of " + serverName + " on " + this.master.getServerName());
     long pid = master.getAssignmentManager().submitServerCrash(serverName, true, force);
+    if (pid == Procedure.NO_PROC_ID) {
+      // skip later processing as we failed to submit SCP
+      return Procedure.NO_PROC_ID;
+    }
     storage.expired(serverName);
     // Tell our listeners that a server was removed
     if (!this.listeners.isEmpty()) {

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

@@ -331,6 +331,7 @@ public void start() throws IOException, KeeperException {
               regionNode.getProcedure().stateLoaded(this, regionNode);
             }
             if (regionLocation != null) {
+              regionStates.createServer(regionLocation);
               regionStates.addRegionToServer(regionNode);
             }
             if (RegionReplicaUtil.isDefaultReplica(regionInfo.getReplicaId())) {
@@ -1108,7 +1109,7 @@ public void deleteTable(final TableName tableName) throws IOException {
   // RS Region Transition Report helpers
   // ============================================================================================
   private void reportRegionStateTransition(ReportRegionStateTransitionResponse.Builder builder,
-    ServerName serverName, List<RegionStateTransition> transitionList) throws IOException {
+    ServerStateNode serverNode, List<RegionStateTransition> transitionList) throws IOException {
     for (RegionStateTransition transition : transitionList) {
       switch (transition.getTransitionCode()) {
         case OPENED:
@@ -1118,7 +1119,7 @@ private void reportRegionStateTransition(ReportRegionStateTransitionResponse.Bui
           final RegionInfo hri = ProtobufUtil.toRegionInfo(transition.getRegionInfo(0));
           long procId =
             transition.getProcIdCount() > 0 ? transition.getProcId(0) : Procedure.NO_PROC_ID;
-          updateRegionTransition(serverName, transition.getTransitionCode(), hri,
+          updateRegionTransition(serverNode, transition.getTransitionCode(), hri,
             transition.hasOpenSeqNum() ? transition.getOpenSeqNum() : HConstants.NO_SEQNUM, procId);
           break;
         case READY_TO_SPLIT:
@@ -1128,7 +1129,7 @@ private void reportRegionStateTransition(ReportRegionStateTransitionResponse.Bui
           final RegionInfo parent = ProtobufUtil.toRegionInfo(transition.getRegionInfo(0));
           final RegionInfo splitA = ProtobufUtil.toRegionInfo(transition.getRegionInfo(1));
           final RegionInfo splitB = ProtobufUtil.toRegionInfo(transition.getRegionInfo(2));
-          updateRegionSplitTransition(serverName, transition.getTransitionCode(), parent, splitA,
+          updateRegionSplitTransition(serverNode, transition.getTransitionCode(), parent, splitA,
             splitB);
           break;
         case READY_TO_MERGE:
@@ -1138,7 +1139,7 @@ private void reportRegionStateTransition(ReportRegionStateTransitionResponse.Bui
           final RegionInfo merged = ProtobufUtil.toRegionInfo(transition.getRegionInfo(0));
           final RegionInfo mergeA = ProtobufUtil.toRegionInfo(transition.getRegionInfo(1));
           final RegionInfo mergeB = ProtobufUtil.toRegionInfo(transition.getRegionInfo(2));
-          updateRegionMergeTransition(serverName, transition.getTransitionCode(), merged, mergeA,
+          updateRegionMergeTransition(serverNode, transition.getTransitionCode(), merged, mergeA,
             mergeB);
           break;
       }
@@ -1150,7 +1151,10 @@ public ReportRegionStateTransitionResponse reportRegionStateTransition(
     ReportRegionStateTransitionResponse.Builder builder =
       ReportRegionStateTransitionResponse.newBuilder();
     ServerName serverName = ProtobufUtil.toServerName(req.getServer());
-    ServerStateNode serverNode = regionStates.getOrCreateServer(serverName);
+    ServerStateNode serverNode = regionStates.getServerNode(serverName);
+    if (serverNode == null) {
+
+    }
     // here we have to acquire a read lock instead of a simple exclusive lock. This is because that
     // we should not block other reportRegionStateTransition call from the same region server. This
     // is not only about performance, but also to prevent dead lock. Think of the meta region is
@@ -1163,7 +1167,7 @@ public ReportRegionStateTransitionResponse reportRegionStateTransition(
       // above in submitServerCrash method and HBASE-21508 for more details.
       if (serverNode.isInState(ServerState.ONLINE)) {
         try {
-          reportRegionStateTransition(builder, serverName, req.getTransitionList());
+          reportRegionStateTransition(builder, serverNode, req.getTransitionList());
         } catch (PleaseHoldException e) {
           LOG.trace("Failed transition ", e);
           throw e;
@@ -1185,21 +1189,20 @@ public ReportRegionStateTransitionResponse reportRegionStateTransition(
     return builder.build();
   }
 
-  private void updateRegionTransition(ServerName serverName, TransitionCode state,
+  private void updateRegionTransition(ServerStateNode serverNode, TransitionCode state,
     RegionInfo regionInfo, long seqId, long procId) throws IOException {
     checkMetaLoaded(regionInfo);
 
     RegionStateNode regionNode = regionStates.getRegionStateNode(regionInfo);
     if (regionNode == null) {
       // the table/region is gone. maybe a delete, split, merge
       throw new UnexpectedStateException(String.format(
-        "Server %s was trying to transition region %s to %s. but Region is not known.", serverName,
-        regionInfo, state));
+        "Server %s was trying to transition region %s to %s. but Region is not known.",
+        serverNode.getServerName(), regionInfo, state));
     }
-    LOG.trace("Update region transition serverName={} region={} regionState={}", serverName,
-      regionNode, state);
+    LOG.trace("Update region transition serverName={} region={} regionState={}",
+      serverNode.getServerName(), regionNode, state);
 
-    ServerStateNode serverNode = regionStates.getOrCreateServer(serverName);
     regionNode.lock();
     try {
       if (!reportTransition(regionNode, serverNode, state, seqId, procId)) {
@@ -1216,8 +1219,8 @@ private void updateRegionTransition(ServerName serverName, TransitionCode state,
         ) {
           LOG.info("RegionServer {} {}", state, regionNode.getRegionInfo().getEncodedName());
         } else {
-          LOG.warn("No matching procedure found for {} transition on {} to {}", serverName,
-            regionNode, state);
+          LOG.warn("No matching procedure found for {} transition on {} to {}",
+            serverNode.getServerName(), regionNode, state);
         }
       }
     } finally {
@@ -1237,8 +1240,9 @@ private boolean reportTransition(RegionStateNode regionNode, ServerStateNode ser
     return true;
   }
 
-  private void updateRegionSplitTransition(final ServerName serverName, final TransitionCode state,
-    final RegionInfo parent, final RegionInfo hriA, final RegionInfo hriB) throws IOException {
+  private void updateRegionSplitTransition(final ServerStateNode serverNode,
+    final TransitionCode state, final RegionInfo parent, final RegionInfo hriA,
+    final RegionInfo hriB) throws IOException {
     checkMetaLoaded(parent);
 
     if (state != TransitionCode.READY_TO_SPLIT) {
@@ -1262,8 +1266,8 @@ private void updateRegionSplitTransition(final ServerName serverName, final Tran
     // Submit the Split procedure
     final byte[] splitKey = hriB.getStartKey();
     if (LOG.isDebugEnabled()) {
-      LOG.debug("Split request from " + serverName + ", parent=" + parent + " splitKey="
-        + Bytes.toStringBinary(splitKey));
+      LOG.debug("Split request from {}, parent={}, splitKey={}", serverNode.getServerName(), parent,
+        Bytes.toStringBinary(splitKey));
     }
     // Processing this report happens asynchronously from other activities which can mutate
     // the region state. For example, a split procedure may already be running for this parent.
@@ -1277,21 +1281,22 @@ private void updateRegionSplitTransition(final ServerName serverName, final Tran
     if (parentState != null && parentState.isOpened()) {
       master.getMasterProcedureExecutor().submitProcedure(createSplitProcedure(parent, splitKey));
     } else {
-      LOG.info("Ignoring split request from " + serverName + ", parent=" + parent
-        + " because parent is unknown or not open");
+      LOG.info("Ignoring split request from {}, parent={} because parent is unknown or not open",
+        serverNode.getServerName(), parent);
       return;
     }
 
     // If the RS is < 2.0 throw an exception to abort the operation, we are handling the split
-    if (master.getServerManager().getVersionNumber(serverName) < 0x0200000) {
+    if (master.getServerManager().getVersionNumber(serverNode.getServerName()) < 0x0200000) {
       throw new UnsupportedOperationException(
         String.format("Split handled by the master: " + "parent=%s hriA=%s hriB=%s",
           parent.getShortNameToLog(), hriA, hriB));
     }
   }
 
-  private void updateRegionMergeTransition(final ServerName serverName, final TransitionCode state,
-    final RegionInfo merged, final RegionInfo hriA, final RegionInfo hriB) throws IOException {
+  private void updateRegionMergeTransition(final ServerStateNode serverNode,
+    final TransitionCode state, final RegionInfo merged, final RegionInfo hriA,
+    final RegionInfo hriB) throws IOException {
     checkMetaLoaded(merged);
 
     if (state != TransitionCode.READY_TO_MERGE) {
@@ -1313,7 +1318,7 @@ private void updateRegionMergeTransition(final ServerName serverName, final Tran
     master.getMasterProcedureExecutor().submitProcedure(createMergeProcedure(hriA, hriB));
 
     // If the RS is < 2.0 throw an exception to abort the operation, we are handling the merge
-    if (master.getServerManager().getVersionNumber(serverName) < 0x0200000) {
+    if (master.getServerManager().getVersionNumber(serverNode.getServerName()) < 0x0200000) {
       throw new UnsupportedOperationException(
         String.format("Merge not handled yet: regionState=%s merged=%s hriA=%s hriB=%s", state,
           merged, hriA, hriB));
@@ -1343,12 +1348,19 @@ public void reportOnlineRegions(ServerName serverName, Set<byte[]> regionNames)
         regionNames.stream().map(Bytes::toStringBinary).collect(Collectors.toList()));
     }
 
-    ServerStateNode serverNode = regionStates.getOrCreateServer(serverName);
-    synchronized (serverNode) {
+    ServerStateNode serverNode = regionStates.getServerNode(serverName);
+    if (serverNode == null) {
+      LOG.warn("Got a report from server {} where its server node is null", serverName);
+      return;
+    }
+    serverNode.readLock().lock();
+    try {
       if (!serverNode.isInState(ServerState.ONLINE)) {
-        LOG.warn("Got a report from a server result in state " + serverNode.getState());
+        LOG.warn("Got a report from a server result in state {}", serverNode);
         return;
       }
+    } finally {
+      serverNode.readLock().unlock();
     }
 
     // Track the regionserver reported online regions in memory.
@@ -1839,52 +1851,60 @@ public long submitServerCrash(ServerName serverName, boolean shouldSplitWal, boo
     synchronized (rsReports) {
       rsReports.remove(serverName);
     }
+    if (serverNode == null) {
+      if (force) {
+        LOG.info("Force adding ServerCrashProcedure for {} when server node is null", serverName);
+      } else {
+        // for normal case, do not schedule SCP if ServerStateNode is null
+        LOG.warn("Skip adding ServerCrashProcedure for {} because server node is null", serverName);
+        return Procedure.NO_PROC_ID;
+      }
+    }
 
+    ProcedureExecutor<MasterProcedureEnv> procExec = this.master.getMasterProcedureExecutor();
     // We hold the write lock here for fencing on reportRegionStateTransition. Once we set the
     // server state to CRASHED, we will no longer accept the reportRegionStateTransition call from
     // this server. This is used to simplify the implementation for TRSP and SCP, where we can make
     // sure that, the region list fetched by SCP will not be changed any more.
     if (serverNode != null) {
       serverNode.writeLock().lock();
     }
-    boolean carryingMeta;
-    long pid;
     try {
-      ProcedureExecutor<MasterProcedureEnv> procExec = this.master.getMasterProcedureExecutor();
-      carryingMeta = isCarryingMeta(serverName);
-      if (!force && serverNode != null && !serverNode.isInState(ServerState.ONLINE)) {
-        LOG.info("Skip adding ServerCrashProcedure for {} (meta={}) -- running?", serverNode,
-          carryingMeta);
-        return Procedure.NO_PROC_ID;
-      } else {
-        MasterProcedureEnv mpe = procExec.getEnvironment();
-        // If serverNode == null, then 'Unknown Server'. Schedule HBCKSCP instead.
-        // HBCKSCP scours Master in-memory state AND hbase;meta for references to
-        // serverName just-in-case. An SCP that is scheduled when the server is
-        // 'Unknown' probably originated externally with HBCK2 fix-it tool.
-        ServerState oldState = null;
-        if (serverNode != null) {
-          oldState = serverNode.getState();
-          serverNode.setState(ServerState.CRASHED);
-        }
 
+      boolean carryingMeta = isCarryingMeta(serverName);
+      if (serverNode != null && !serverNode.isInState(ServerState.ONLINE)) {
         if (force) {
-          pid = procExec.submitProcedure(
-            new HBCKServerCrashProcedure(mpe, serverName, shouldSplitWal, carryingMeta));
+          LOG.info("Force adding ServerCrashProcedure for {} (meta={}) when state is not {}",
+            serverNode, carryingMeta, ServerState.ONLINE);
         } else {
-          pid = procExec.submitProcedure(
-            new ServerCrashProcedure(mpe, serverName, shouldSplitWal, carryingMeta));
+          LOG.info("Skip adding ServerCrashProcedure for {} (meta={}) when state is not {}",
+            serverNode, carryingMeta, ServerState.ONLINE);
+          return Procedure.NO_PROC_ID;
         }
-        LOG.info("Scheduled ServerCrashProcedure pid={} for {} (carryingMeta={}){}.", pid,
-          serverName, carryingMeta,
-          serverNode == null ? "" : " " + serverNode.toString() + ", oldState=" + oldState);
       }
+      MasterProcedureEnv mpe = procExec.getEnvironment();
+      // If serverNode == null, then 'Unknown Server'. Schedule HBCKSCP instead.
+      // HBCKSCP scours Master in-memory state AND hbase;meta for references to
+      // serverName just-in-case. An SCP that is scheduled when the server is
+      // 'Unknown' probably originated externally with HBCK2 fix-it tool.
+      ServerState oldState = null;
+      if (serverNode != null) {
+        oldState = serverNode.getState();
+        serverNode.setState(ServerState.CRASHED);
+      }
+      ServerCrashProcedure scp = force
+        ? new HBCKServerCrashProcedure(mpe, serverName, shouldSplitWal, carryingMeta)
+        : new ServerCrashProcedure(mpe, serverName, shouldSplitWal, carryingMeta);
+      long pid = procExec.submitProcedure(scp);
+      LOG.info("Scheduled ServerCrashProcedure pid={} for {} (carryingMeta={}){}.", pid, serverName,
+        carryingMeta,
+        serverNode == null ? "" : " " + serverNode.toString() + ", oldState=" + oldState);
+      return pid;
     } finally {
       if (serverNode != null) {
         serverNode.writeLock().unlock();
       }
     }
-    return pid;
   }
 
   public void offlineRegion(final RegionInfo regionInfo) {

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

@@ -405,7 +405,7 @@ private boolean include(final RegionStateNode node, final boolean offline) {
   // ============================================================================================
 
   private void setServerState(ServerName serverName, ServerState state) {
-    ServerStateNode serverNode = getOrCreateServer(serverName);
+    ServerStateNode serverNode = getServerNode(serverName);
     synchronized (serverNode) {
       serverNode.setState(state);
     }
@@ -746,18 +746,16 @@ public List<RegionState> getRegionFailedOpen() {
   // ==========================================================================
 
   /**
-   * Be judicious calling this method. Do it on server register ONLY otherwise you could mess up
-   * online server accounting. TOOD: Review usage and convert to {@link #getServerNode(ServerName)}
-   * where we can.
+   * Create the ServerStateNode when registering a new region server
    */
-  public ServerStateNode getOrCreateServer(final ServerName serverName) {
-    return serverMap.computeIfAbsent(serverName, key -> new ServerStateNode(key));
+  public void createServer(ServerName serverName) {
+    serverMap.computeIfAbsent(serverName, key -> new ServerStateNode(key));
   }
 
   /**
    * Called by SCP at end of successful processing.
    */
-  public void removeServer(final ServerName serverName) {
+  public void removeServer(ServerName serverName) {
     serverMap.remove(serverName);
   }
 
@@ -777,14 +775,14 @@ public double getAverageLoad() {
   }
 
   public ServerStateNode addRegionToServer(final RegionStateNode regionNode) {
-    ServerStateNode serverNode = getOrCreateServer(regionNode.getRegionLocation());
+    ServerStateNode serverNode = getServerNode(regionNode.getRegionLocation());
     serverNode.addRegion(regionNode);
     return serverNode;
   }
 
   public ServerStateNode removeRegionFromServer(final ServerName serverName,
     final RegionStateNode regionNode) {
-    ServerStateNode serverNode = getOrCreateServer(serverName);
+    ServerStateNode serverNode = getServerNode(serverName);
     serverNode.removeRegion(regionNode);
     return serverNode;
   }

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

@@ -78,7 +78,7 @@ public void testAssignmentsForBalancer() throws Exception {
     AssignmentManager assignmentManager = master.getAssignmentManager();
     RegionStates regionStates = assignmentManager.getRegionStates();
     ServerName sn1 = ServerName.parseServerName("asf903.gq1.ygridcore.net,52690,1517835491385");
-    regionStates.getOrCreateServer(sn1);
+    regionStates.createServer(sn1);
 
     TableStateManager tableStateManager = master.getTableStateManager();
     ServerManager serverManager = master.getServerManager();