HBASE-22365 Region may be opened on two RegionServers

Author: Apache9

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

@@ -213,6 +213,9 @@ public void start() throws IOException, KeeperException {
       try {
         regionNode.setRegionLocation(regionState.getServerName());
         regionNode.setState(regionState.getState());
+        if (regionNode.getProcedure() != null) {
+          regionNode.getProcedure().stateLoaded(this, regionNode);
+        }
         setMetaAssigned(regionState.getRegion(), regionState.getState() == State.OPEN);
       } finally {
         regionNode.unlock();
@@ -235,14 +238,12 @@ public void setupRIT(List<TransitRegionStateProcedure> procs) {
       TransitRegionStateProcedure existingProc = regionNode.getProcedure();
       if (existingProc != null) {
         // This is possible, as we will detach the procedure from the RSN before we
-        // actually finish the procedure. This is because that, we will update the region state
-        // directly in the reportTransition method for TRSP, and theoretically the region transition
-        // has been done, so we need to detach the procedure from the RSN. But actually the
-        // procedure has not been marked as done in the pv2 framework yet, so it is possible that we
-        // schedule a new TRSP immediately and when arriving here, we will find out that there are
-        // multiple TRSPs for the region. But we can make sure that, only the last one can take the
-        // charge, the previous ones should have all been finished already.
-        // So here we will compare the proc id, the greater one will win.
+        // actually finish the procedure. This is because that, we will detach the TRSP from the RSN
+        // during execution, at that time, the procedure has not been marked as done in the pv2
+        // framework yet, so it is possible that we schedule a new TRSP immediately and when
+        // arriving here, we will find out that there are multiple TRSPs for the region. But we can
+        // make sure that, only the last one can take the charge, the previous ones should have all
+        // been finished already. So here we will compare the proc id, the greater one will win.
         if (existingProc.getProcId() < proc.getProcId()) {
           // the new one wins, unset and set it to the new one below
           regionNode.unsetProcedure(existingProc);
@@ -1307,7 +1308,6 @@ public void visitRegionState(Result result, final RegionInfo regionInfo, final S
           // In any of these cases, state is empty. For now, presume OFFLINE but there are probably
           // cases where we need to probe more to be sure this correct; TODO informed by experience.
           LOG.info(regionInfo.getEncodedName() + " regionState=null; presuming " + State.OFFLINE);
-
           localState = State.OFFLINE;
         }
         RegionStateNode regionNode = regionStates.getOrCreateRegionStateNode(regionInfo);
@@ -1328,6 +1328,9 @@ public void visitRegionState(Result result, final RegionInfo regionInfo, final S
         } else if (localState == State.OFFLINE || regionInfo.isOffline()) {
           regionStates.addToOfflineRegions(regionNode);
         }
+        if (regionNode.getProcedure() != null) {
+          regionNode.getProcedure().stateLoaded(AssignmentManager.this, regionNode);
+        }
       }
     });
 
@@ -1488,8 +1491,9 @@ public RegionInfo getRegionInfo(final byte[] regionName) {
   private static final State[] STATES_EXPECTED_ON_UNASSIGN_OR_MOVE = { State.OPEN };
 
   // ============================================================================================
-  //  Region Status update
-  //  Should only be called in TransitRegionStateProcedure
+  // Region Status update
+  // Should only be called in TransitRegionStateProcedure(and related procedures), as the locking
+  // and pre-assumptions are very tricky.
   // ============================================================================================
   private void transitStateAndUpdate(RegionStateNode regionNode, RegionState.State newState,
       RegionState.State... expectedStates) throws IOException {
@@ -1518,7 +1522,7 @@ void regionOpening(RegionStateNode regionNode) throws IOException {
     metrics.incrementOperationCounter();
   }
 
-  // should be called within the synchronized block of RegionStateNode.
+  // should be called under the RegionStateNode lock
   // The parameter 'giveUp' means whether we will try to open the region again, if it is true, then
   // we will persist the FAILED_OPEN state into hbase:meta.
   void regionFailedOpen(RegionStateNode regionNode, boolean giveUp) throws IOException {
@@ -1544,24 +1548,7 @@ void regionFailedOpen(RegionStateNode regionNode, boolean giveUp) throws IOExcep
     }
   }
 
-  // should be called within the synchronized block of RegionStateNode
-  void regionOpened(RegionStateNode regionNode) throws IOException {
-    // TODO: OPENING Updates hbase:meta too... we need to do both here and there?
-    // That is a lot of hbase:meta writing.
-    transitStateAndUpdate(regionNode, State.OPEN, STATES_EXPECTED_ON_OPEN);
-    RegionInfo hri = regionNode.getRegionInfo();
-    if (isMetaRegion(hri)) {
-      // Usually we'd set a table ENABLED at this stage but hbase:meta is ALWAYs enabled, it
-      // can't be disabled -- so skip the RPC (besides... enabled is managed by TableStateManager
-      // which is backed by hbase:meta... Avoid setting ENABLED to avoid having to update state
-      // on table that contains state.
-      setMetaAssigned(hri, true);
-    }
-    regionStates.addRegionToServer(regionNode);
-    regionStates.removeFromFailedOpen(hri);
-  }
-
-  // should be called within the synchronized block of RegionStateNode
+  // should be called under the RegionStateNode lock
   void regionClosing(RegionStateNode regionNode) throws IOException {
     transitStateAndUpdate(regionNode, State.CLOSING, STATES_EXPECTED_ON_CLOSING);
 
@@ -1575,18 +1562,36 @@ void regionClosing(RegionStateNode regionNode) throws IOException {
     metrics.incrementOperationCounter();
   }
 
-  // should be called within the synchronized block of RegionStateNode
-  // The parameter 'normally' means whether we are closed cleanly, if it is true, then it means that
-  // we are closed due to a RS crash.
-  void regionClosed(RegionStateNode regionNode, boolean normally) throws IOException {
-    RegionState.State state = regionNode.getState();
+  // for open and close, they will first be persist to the procedure store in
+  // RegionRemoteProcedureBase. So here we will first change the in memory state as it is considered
+  // as succeeded if the persistence to procedure store is succeeded, and then when the
+  // RegionRemoteProcedureBase is woken up, we will persist the RegionStateNode to hbase:meta.
+
+  // should be called under the RegionStateNode lock
+  void regionOpenedWithoutPersistingToMeta(RegionStateNode regionNode) throws IOException {
+    regionNode.transitionState(State.OPEN, STATES_EXPECTED_ON_OPEN);
+    RegionInfo regionInfo = regionNode.getRegionInfo();
+    regionStates.addRegionToServer(regionNode);
+    regionStates.removeFromFailedOpen(regionInfo);
+  }
+
+  // should be called under the RegionStateNode lock
+  void regionClosedWithoutPersistingToMeta(RegionStateNode regionNode) throws IOException {
     ServerName regionLocation = regionNode.getRegionLocation();
-    if (normally) {
-      regionNode.transitionState(State.CLOSED, STATES_EXPECTED_ON_CLOSED);
-    } else {
-      // For SCP
-      regionNode.transitionState(State.ABNORMALLY_CLOSED);
+    regionNode.transitionState(State.CLOSED, STATES_EXPECTED_ON_CLOSED);
+    regionNode.setRegionLocation(null);
+    if (regionLocation != null) {
+      regionNode.setLastHost(regionLocation);
+      regionStates.removeRegionFromServer(regionLocation, regionNode);
     }
+  }
+
+  // should be called under the RegionStateNode lock
+  // for SCP
+  void regionClosedAbnormally(RegionStateNode regionNode) throws IOException {
+    RegionState.State state = regionNode.getState();
+    ServerName regionLocation = regionNode.getRegionLocation();
+    regionNode.transitionState(State.ABNORMALLY_CLOSED);
     regionNode.setRegionLocation(null);
     boolean succ = false;
     try {
@@ -1605,6 +1610,22 @@ void regionClosed(RegionStateNode regionNode, boolean normally) throws IOExcepti
     }
   }
 
+  void persistToMeta(RegionStateNode regionNode) throws IOException {
+    regionStateStore.updateRegionLocation(regionNode);
+    RegionInfo regionInfo = regionNode.getRegionInfo();
+    if (isMetaRegion(regionInfo) && regionNode.getState() == State.OPEN) {
+      // Usually we'd set a table ENABLED at this stage but hbase:meta is ALWAYs enabled, it
+      // can't be disabled -- so skip the RPC (besides... enabled is managed by TableStateManager
+      // which is backed by hbase:meta... Avoid setting ENABLED to avoid having to update state
+      // on table that contains state.
+      setMetaAssigned(regionInfo, true);
+    }
+  }
+
+  // ============================================================================================
+  // The above methods can only be called in TransitRegionStateProcedure(and related procedures)
+  // ============================================================================================
+
   public void markRegionAsSplit(final RegionInfo parent, final ServerName serverName,
       final RegionInfo daughterA, final RegionInfo daughterB) throws IOException {
     // Update hbase:meta. Parent will be marked offline and split up in hbase:meta.

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

@@ -21,6 +21,7 @@
 import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.client.RegionInfo;
 import org.apache.hadoop.hbase.exceptions.UnexpectedStateException;
+import org.apache.hadoop.hbase.master.RegionState.State;
 import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
 import org.apache.hadoop.hbase.master.procedure.RSProcedureDispatcher.RegionCloseOperation;
 import org.apache.hadoop.hbase.procedure2.ProcedureMetrics;
@@ -90,17 +91,28 @@ protected ProcedureMetrics getProcedureMetrics(MasterProcedureEnv env) {
   }
 
   @Override
-  protected void reportTransition(RegionStateNode regionNode, TransitionCode transitionCode,
-      long seqId) throws IOException {
+  protected void checkTransition(RegionStateNode regionNode, TransitionCode transitionCode,
+      long seqId) throws UnexpectedStateException {
     if (transitionCode != TransitionCode.CLOSED) {
       throw new UnexpectedStateException("Received report unexpected " + transitionCode +
         " transition, " + regionNode.toShortString() + ", " + this + ", expected CLOSED.");
     }
   }
 
   @Override
-  protected void updateTransition(MasterProcedureEnv env, RegionStateNode regionNode,
-      TransitionCode transitionCode, long seqId) throws IOException {
-    env.getAssignmentManager().regionClosed(regionNode, true);
+  protected void updateTransitionWithoutPersistingToMeta(MasterProcedureEnv env,
+      RegionStateNode regionNode, TransitionCode transitionCode, long seqId) throws IOException {
+    assert transitionCode == TransitionCode.CLOSED;
+    env.getAssignmentManager().regionClosedWithoutPersistingToMeta(regionNode);
+  }
+
+  @Override
+  protected void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode, long seqId)
+      throws IOException {
+    if (regionNode.getState() == State.CLOSED) {
+      // should have already been persisted, ignore
+      return;
+    }
+    am.regionClosedWithoutPersistingToMeta(regionNode);
   }
 }

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

@@ -21,6 +21,7 @@
 import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.client.RegionInfo;
 import org.apache.hadoop.hbase.exceptions.UnexpectedStateException;
+import org.apache.hadoop.hbase.master.RegionState.State;
 import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
 import org.apache.hadoop.hbase.master.procedure.RSProcedureDispatcher.RegionOpenOperation;
 import org.apache.hadoop.hbase.procedure2.ProcedureMetrics;
@@ -77,19 +78,31 @@ protected ProcedureMetrics getProcedureMetrics(MasterProcedureEnv env) {
     return env.getAssignmentManager().getAssignmentManagerMetrics().getOpenProcMetrics();
   }
 
+  private void regionOpenedWithoutPersistingToMeta(AssignmentManager am, RegionStateNode regionNode,
+      TransitionCode transitionCode, long openSeqNum) throws IOException {
+    if (openSeqNum < regionNode.getOpenSeqNum()) {
+      LOG.warn(
+        "Received report {} transition from {} for {}, pid={} but the new openSeqNum {}" +
+          " is less than the current one {}, ignoring...",
+        transitionCode, targetServer, regionNode, getProcId(), openSeqNum,
+        regionNode.getOpenSeqNum());
+    } else {
+      regionNode.setOpenSeqNum(openSeqNum);
+    }
+    am.regionOpenedWithoutPersistingToMeta(regionNode);
+  }
+
   @Override
-  protected void reportTransition(RegionStateNode regionNode, TransitionCode transitionCode,
-      long seqId) throws IOException {
+  protected void checkTransition(RegionStateNode regionNode, TransitionCode transitionCode,
+      long openSeqNum) throws UnexpectedStateException {
     switch (transitionCode) {
       case OPENED:
-        // this is the openSeqNum
-        if (seqId < 0) {
+        if (openSeqNum < 0) {
           throw new UnexpectedStateException("Received report unexpected " + TransitionCode.OPENED +
-            " transition openSeqNum=" + seqId + ", " + regionNode + ", proc=" + this);
+            " transition openSeqNum=" + openSeqNum + ", " + regionNode + ", proc=" + this);
         }
         break;
       case FAILED_OPEN:
-        // nothing to check
         break;
       default:
         throw new UnexpectedStateException(
@@ -99,27 +112,26 @@ protected void reportTransition(RegionStateNode regionNode, TransitionCode trans
   }
 
   @Override
-  protected void updateTransition(MasterProcedureEnv env, RegionStateNode regionNode,
-      TransitionCode transitionCode, long openSeqNum) throws IOException {
-    switch (transitionCode) {
-      case OPENED:
-        if (openSeqNum < regionNode.getOpenSeqNum()) {
-          LOG.warn(
-            "Received report {} transition from {} for {}, pid={} but the new openSeqNum {}" +
-              " is less than the current one {}, ignoring...",
-            transitionCode, targetServer, regionNode, getProcId(), openSeqNum,
-            regionNode.getOpenSeqNum());
-        } else {
-          regionNode.setOpenSeqNum(openSeqNum);
-        }
-        env.getAssignmentManager().regionOpened(regionNode);
-        break;
-      case FAILED_OPEN:
-        env.getAssignmentManager().regionFailedOpen(regionNode, false);
-        break;
-      default:
-        throw new UnexpectedStateException("Unexpected transition code: " + transitionCode);
+  protected void updateTransitionWithoutPersistingToMeta(MasterProcedureEnv env,
+      RegionStateNode regionNode, TransitionCode transitionCode, long openSeqNum)
+      throws IOException {
+    if (transitionCode == TransitionCode.OPENED) {
+      regionOpenedWithoutPersistingToMeta(env.getAssignmentManager(), regionNode, transitionCode,
+        openSeqNum);
+    } else {
+      assert transitionCode == TransitionCode.FAILED_OPEN;
+      // will not persist to meta if giveUp is false
+      env.getAssignmentManager().regionFailedOpen(regionNode, false);
     }
+  }
 
+  @Override
+  protected void restoreSucceedState(AssignmentManager am, RegionStateNode regionNode,
+      long openSeqNum) throws IOException {
+    if (regionNode.getState() == State.OPEN) {
+      // should have already been persisted, ignore
+      return;
+    }
+    regionOpenedWithoutPersistingToMeta(am, regionNode, TransitionCode.OPENED, openSeqNum);
   }
 }