HBASE-22404 Open/Close region request may be executed twice when master restart

Author: infraio

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java

@@ -51,6 +51,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.ConcurrentSkipListMap;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.function.Function;
@@ -181,6 +182,8 @@
 import org.apache.hadoop.hbase.zookeeper.ZNodePaths;
 import org.apache.hadoop.ipc.RemoteException;
 import org.apache.hadoop.util.ReflectionUtils;
+import org.apache.hbase.thirdparty.com.google.common.cache.Cache;
+import org.apache.hbase.thirdparty.com.google.common.cache.CacheBuilder;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.apache.zookeeper.KeeperException;
 import org.slf4j.Logger;
@@ -255,6 +258,16 @@ public class HRegionServer extends HasThread implements
   protected final ConcurrentMap<byte[], Boolean> regionsInTransitionInRS =
     new ConcurrentSkipListMap<>(Bytes.BYTES_COMPARATOR);
 
+  /**
+   * Used to cache the open/close region procedures which already submitted.
+   */
+  private final ConcurrentMap<Long, Long> submittedRegionProcedures = new ConcurrentHashMap<>();
+  /**
+   * Used to cache the open/close region procedures which already executed.
+   */
+  private final Cache<Long, Long> executedRegionProcedures =
+      CacheBuilder.newBuilder().expireAfterAccess(600, TimeUnit.SECONDS).build();
+
   // Cache flushing
   protected MemStoreFlusher cacheFlusher;
 
@@ -3882,6 +3895,35 @@ void reportProcedureDone(ReportProcedureDoneRequest request) throws IOException
     }
   }
 
+  /**
+   * Will ignore the open/close region procedures which already submitted or executed.
+   * See HBASE-22404 for more details.
+   * @param procId the id of the open/close region procedure
+   * @return true if the procedure can be submitted.
+   */
+  boolean submitRegionProcedure(long procId) {
+    if (procId == -1) {
+      return true;
+    }
+    // Ignore the region procedures which already submitted.
+    Long previous = submittedRegionProcedures.putIfAbsent(procId, procId);
+    if (previous != null) {
+      LOG.warn("Received procedure pid={}, which already submitted, just ignore it", procId);
+      return false;
+    }
+    // Ignore the region procedures which already executed.
+    if (executedRegionProcedures.getIfPresent(procId) != null) {
+      LOG.warn("Received procedure pid={}, which already executed, just ignore it", procId);
+      return false;
+    }
+    return true;
+  }
+
+  public void finishRegionProcedure(long procId) {
+    submittedRegionProcedures.remove(procId);
+    executedRegionProcedures.put(procId, procId);
+  }
+
   public boolean isShutDown() {
     return shutDown;
   }

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java

@@ -3721,8 +3721,12 @@ private void executeOpenRegionProcedures(OpenRegionRequest request,
         regionServer.updateRegionFavoredNodesMapping(regionInfo.getEncodedName(),
           regionOpenInfo.getFavoredNodesList());
       }
-      regionServer.executorService.submit(AssignRegionHandler.create(regionServer, regionInfo,
-        regionOpenInfo.getOpenProcId(), tableDesc, masterSystemTime));
+      long procId = regionOpenInfo.getOpenProcId();
+      if (regionServer.submitRegionProcedure(procId)) {
+        regionServer.executorService.submit(AssignRegionHandler
+            .create(regionServer, regionInfo, procId, tableDesc,
+                masterSystemTime));
+      }
     }
   }
 
@@ -3733,11 +3737,14 @@ private void executeCloseRegionProcedures(CloseRegionRequest request) {
     } catch (DoNotRetryIOException e) {
       throw new UncheckedIOException("Should not happen", e);
     }
-    ServerName destination =
-      request.hasDestinationServer() ? ProtobufUtil.toServerName(request.getDestinationServer())
-        : null;
-    regionServer.executorService.submit(UnassignRegionHandler.create(regionServer, encodedName,
-      request.getCloseProcId(), false, destination));
+    ServerName destination = request.hasDestinationServer() ?
+        ProtobufUtil.toServerName(request.getDestinationServer()) :
+        null;
+    long procId = request.getCloseProcId();
+    if (regionServer.submitRegionProcedure(procId)) {
+      regionServer.executorService.submit(UnassignRegionHandler
+          .create(regionServer, encodedName, procId, false, destination));
+    }
   }
 
   private void executeProcedures(RemoteProcedureRequest request) {

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/AssignRegionHandler.java

@@ -26,6 +26,7 @@
 import org.apache.hadoop.hbase.executor.EventHandler;
 import org.apache.hadoop.hbase.executor.EventType;
 import org.apache.hadoop.hbase.regionserver.HRegion;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
 import org.apache.hadoop.hbase.regionserver.Region;
 import org.apache.hadoop.hbase.regionserver.RegionServerServices;
 import org.apache.hadoop.hbase.regionserver.RegionServerServices.PostOpenDeployContext;
@@ -59,7 +60,7 @@ public class AssignRegionHandler extends EventHandler {
 
   private final RetryCounter retryCounter;
 
-  public AssignRegionHandler(RegionServerServices server, RegionInfo regionInfo, long openProcId,
+  public AssignRegionHandler(HRegionServer server, RegionInfo regionInfo, long openProcId,
       @Nullable TableDescriptor tableDesc, long masterSystemTime, EventType eventType) {
     super(server, eventType);
     this.regionInfo = regionInfo;
@@ -69,8 +70,8 @@ public AssignRegionHandler(RegionServerServices server, RegionInfo regionInfo, l
     this.retryCounter = HandlerUtil.getRetryCounter();
   }
 
-  private RegionServerServices getServer() {
-    return (RegionServerServices) server;
+  private HRegionServer getServer() {
+    return (HRegionServer) server;
   }
 
   private void cleanUpAndReportFailure(IOException error) throws IOException {
@@ -87,7 +88,7 @@ private void cleanUpAndReportFailure(IOException error) throws IOException {
 
   @Override
   public void process() throws IOException {
-    RegionServerServices rs = getServer();
+    HRegionServer rs = getServer();
     String encodedName = regionInfo.getEncodedName();
     byte[] encodedNameBytes = regionInfo.getEncodedNameAsBytes();
     String regionName = regionInfo.getRegionNameAsString();
@@ -101,7 +102,6 @@ public void process() throws IOException {
       // reportRegionStateTransition any more.
       return;
     }
-    LOG.info("Open {}", regionName);
     Boolean previous = rs.getRegionsInTransitionInRS().putIfAbsent(encodedNameBytes, Boolean.TRUE);
     if (previous != null) {
       if (previous) {
@@ -121,6 +121,7 @@ public void process() throws IOException {
       }
       return;
     }
+    LOG.info("Open {}", regionName);
     HRegion region;
     try {
       TableDescriptor htd =
@@ -139,6 +140,8 @@ public void process() throws IOException {
     rs.postOpenDeployTasks(new PostOpenDeployContext(region, openProcId, masterSystemTime));
     rs.addRegion(region);
     LOG.info("Opened {}", regionName);
+    // Cache the open region procedure id after report region transition succeed.
+    rs.finishRegionProcedure(openProcId);
     Boolean current = rs.getRegionsInTransitionInRS().remove(regionInfo.getEncodedNameAsBytes());
     if (current == null) {
       // Should NEVER happen, but let's be paranoid.
@@ -158,7 +161,7 @@ protected void handleException(Throwable t) {
       "Failed to open region " + regionInfo.getRegionNameAsString() + " and can not recover", t);
   }
 
-  public static AssignRegionHandler create(RegionServerServices server, RegionInfo regionInfo,
+  public static AssignRegionHandler create(HRegionServer server, RegionInfo regionInfo,
       long openProcId, TableDescriptor tableDesc, long masterSystemTime) {
     EventType eventType;
     if (regionInfo.isMetaRegion()) {

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/UnassignRegionHandler.java

@@ -25,6 +25,7 @@
 import org.apache.hadoop.hbase.executor.EventHandler;
 import org.apache.hadoop.hbase.executor.EventType;
 import org.apache.hadoop.hbase.regionserver.HRegion;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
 import org.apache.hadoop.hbase.regionserver.Region;
 import org.apache.hadoop.hbase.regionserver.RegionServerServices;
 import org.apache.hadoop.hbase.regionserver.RegionServerServices.RegionStateTransitionContext;
@@ -60,7 +61,7 @@ public class UnassignRegionHandler extends EventHandler {
 
   private final RetryCounter retryCounter;
 
-  public UnassignRegionHandler(RegionServerServices server, String encodedName, long closeProcId,
+  public UnassignRegionHandler(HRegionServer server, String encodedName, long closeProcId,
       boolean abort, @Nullable ServerName destination, EventType eventType) {
     super(server, eventType);
     this.encodedName = encodedName;
@@ -70,13 +71,13 @@ public UnassignRegionHandler(RegionServerServices server, String encodedName, lo
     this.retryCounter = HandlerUtil.getRetryCounter();
   }
 
-  private RegionServerServices getServer() {
-    return (RegionServerServices) server;
+  private HRegionServer getServer() {
+    return (HRegionServer) server;
   }
 
   @Override
   public void process() throws IOException {
-    RegionServerServices rs = getServer();
+    HRegionServer rs = getServer();
     byte[] encodedNameBytes = Bytes.toBytes(encodedName);
     Boolean previous = rs.getRegionsInTransitionInRS().putIfAbsent(encodedNameBytes, Boolean.FALSE);
     if (previous != null) {
@@ -94,7 +95,7 @@ public void process() throws IOException {
       }
       return;
     }
-    HRegion region = (HRegion) rs.getRegion(encodedName);
+    HRegion region = rs.getRegion(encodedName);
     if (region == null) {
       LOG.debug(
         "Received CLOSE for a region {} which is not online, and we're not opening/closing.",
@@ -124,6 +125,8 @@ public void process() throws IOException {
       HConstants.NO_SEQNUM, closeProcId, -1, region.getRegionInfo()))) {
       throw new IOException("Failed to report close to master: " + regionName);
     }
+    // Cache the close region procedure id after report region transition succeed.
+    rs.finishRegionProcedure(closeProcId);
     rs.getRegionsInTransitionInRS().remove(encodedNameBytes, Boolean.FALSE);
     LOG.info("Closed {}", regionName);
   }
@@ -134,7 +137,7 @@ protected void handleException(Throwable t) {
     getServer().abort("Failed to close region " + encodedName + " and can not recover", t);
   }
 
-  public static UnassignRegionHandler create(RegionServerServices server, String encodedName,
+  public static UnassignRegionHandler create(HRegionServer server, String encodedName,
       long closeProcId, boolean abort, @Nullable ServerName destination) {
     // Just try our best to determine whether it is for closing meta. It is not the end of the world
     // if we put the handler into a wrong executor.