HBASE-7790 Refactor OpenRegionHandler so that the cleanup happens in one place - the finally block(Ram)

git-svn-id: https://svn.apache.org/repos/asf/hbase/trunk@1450269 13f79535-47bb-0310-9956-ffa450edef68

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

@@ -32,6 +32,7 @@ import org.apache.hadoop.hbase.executor.EventType;
 import org.apache.hadoop.hbase.regionserver.HRegion;
 import org.apache.hadoop.hbase.regionserver.RegionServerAccounting;
 import org.apache.hadoop.hbase.regionserver.RegionServerServices;
+import org.apache.hadoop.hbase.regionserver.wal.HLog;
 import org.apache.hadoop.hbase.util.CancelableProgressable;
 import org.apache.hadoop.hbase.zookeeper.ZKAssign;
 import org.apache.zookeeper.KeeperException;
@@ -87,8 +88,9 @@ public class OpenRegionHandler extends EventHandler {
   @Override
   public void process() throws IOException {
     boolean openSuccessful = false;
-    boolean transitionToFailedOpen = false;
+    boolean transitionedToOpening = false;
     final String regionName = regionInfo.getRegionNameAsString();
+    HRegion region = null;
 
     try {
       if (this.server.isStopped() || this.rsServices.isStopping()) {
@@ -106,7 +108,6 @@ public class OpenRegionHandler extends EventHandler {
         LOG.error("Region " + encodedName +
             " was already online when we started processing the opening. " +
             "Marking this new attempt as failed");
-        tryTransitionFromOfflineToFailedOpen(this.rsServices, regionInfo, this.version);
         return;
       }
 
@@ -115,23 +116,19 @@ public class OpenRegionHandler extends EventHandler {
       // Calling transitionZookeeperOfflineToOpening initializes this.version.
       if (!isRegionStillOpening()){
         LOG.error("Region " + encodedName + " opening cancelled");
-        tryTransitionFromOfflineToFailedOpen(this.rsServices, regionInfo, this.version);
         return;
       }
 
       if (!transitionZookeeperOfflineToOpening(encodedName, versionOfOfflineNode)) {
         LOG.warn("Region was hijacked? Opening cancelled for encodedName=" + encodedName);
         // This is a desperate attempt: the znode is unlikely to be ours. But we can't do more.
-        tryTransitionFromOfflineToFailedOpen(this.rsServices, regionInfo, this.version);
         return;
       }
-
+      transitionedToOpening = true;
       // Open region.  After a successful open, failures in subsequent
       // processing needs to do a close as part of cleanup.
-      HRegion region = openRegion();
+      region = openRegion();
       if (region == null) {
-        tryTransitionFromOpeningToFailedOpen(regionInfo);
-        transitionToFailedOpen = true;
         return;
       }
       boolean failed = true;
@@ -142,9 +139,6 @@ public class OpenRegionHandler extends EventHandler {
       }
       if (failed || this.server.isStopped() ||
           this.rsServices.isStopping()) {
-        cleanupFailedOpen(region);
-        tryTransitionFromOpeningToFailedOpen(regionInfo);
-        transitionToFailedOpen = true;
         return;
       }
 
@@ -155,9 +149,6 @@ public class OpenRegionHandler extends EventHandler {
         // OR (b) someone else opened the region before us
         // OR (c) someone cancelled the open
         // In all cases, we try to transition to failed_open to be safe.
-        cleanupFailedOpen(region);
-        tryTransitionFromOpeningToFailedOpen(regionInfo);
-        transitionToFailedOpen = true;
         return;
       }
 
@@ -180,6 +171,10 @@ public class OpenRegionHandler extends EventHandler {
 
 
     } finally {
+      // Do all clean up here
+      if (!openSuccessful) {
+        doCleanUpOnFailedOpen(region, transitionedToOpening);
+      }
       final Boolean current = this.rsServices.getRegionsInTransitionInRS().
           remove(this.regionInfo.getEncodedNameAsBytes());
 
@@ -191,19 +186,35 @@ public class OpenRegionHandler extends EventHandler {
       // 1) this.rsServices.addToOnlineRegions(region);
       // 2) the ZK state.
       if (openSuccessful) {
-        if (current == null) {  // Should NEVER happen, but let's be paranoid.
+        if (current == null) { // Should NEVER happen, but let's be paranoid.
-          LOG.error("Bad state: we've just opened a region that was NOT in transition. Region=" +
+          LOG.error("Bad state: we've just opened a region that was NOT in transition. Region="
-              regionName
+              + regionName);
-          );
+        } else if (Boolean.FALSE.equals(current)) { // Can happen, if we're
-        } else if (Boolean.FALSE.equals(current)) { // Can happen, if we're really unlucky.
+                                                    // really unlucky.
           LOG.error("Race condition: we've finished to open a region, while a close was requested "
-              + " on region=" + regionName + ". It can be a critical error, as a region that" +
+              + " on region=" + regionName + ". It can be a critical error, as a region that"
-              " should be closed is now opened."
+              + " should be closed is now opened.");
-          );
         }
-      } else if (transitionToFailedOpen == false) {
+      } 
+    }
+  }
+
+  private void doCleanUpOnFailedOpen(HRegion region, boolean transitionedToOpening)
+      throws IOException {
+    if (transitionedToOpening) {
+      try {
+        if (region != null) {
+          cleanupFailedOpen(region);
+        }
+      } finally {
+        // Even if cleanupFailed open fails we need to do this transition
+        // See HBASE-7698
         tryTransitionFromOpeningToFailedOpen(regionInfo);
       }
+    } else {
+      // If still transition to OPENING is not done, we need to transition znode
+      // to FAILED_OPEN
+      tryTransitionFromOfflineToFailedOpen(this.rsServices, regionInfo, versionOfOfflineNode);
     }
   }
 

hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java

@@ -212,5 +212,77 @@ public class TestOpenRegionHandler {
         TEST_HRI.getEncodedName()));
     assertEquals(EventType.RS_ZK_REGION_FAILED_OPEN, rt.getEventType());
   }
+
+  @Test
+  public void testTransitionToFailedOpenFromOffline() throws Exception {
+    Server server = new MockServer(HTU);
+    RegionServerServices rsServices = new MockRegionServerServices(server.getZooKeeper(),
+        server.getServerName());
+    // Create it OFFLINE, which is what it expects
+    ZKAssign.createNodeOffline(server.getZooKeeper(), TEST_HRI, server.getServerName());
+    // Create the handler
+    OpenRegionHandler handler = new OpenRegionHandler(server, rsServices, TEST_HRI, TEST_HTD) {
+
+      @Override
+      boolean transitionZookeeperOfflineToOpening(String encodedName, int versionOfOfflineNode) {
+        return false;
+      }
+    };
+    rsServices.getRegionsInTransitionInRS().put(TEST_HRI.getEncodedNameAsBytes(), Boolean.TRUE);
+
+    handler.process();
+
+    RegionTransition rt = RegionTransition.parseFrom(ZKAssign.getData(server.getZooKeeper(),
+        TEST_HRI.getEncodedName()));
+    assertEquals(EventType.RS_ZK_REGION_FAILED_OPEN, rt.getEventType());
+  }
+
+  @Test
+  public void testTransitionToFailedOpenFromOffline() throws Exception {
+    Server server = new MockServer(HTU);
+    RegionServerServices rsServices = new MockRegionServerServices(server.getZooKeeper(),
+        server.getServerName());
+    // Create it OFFLINE, which is what it expects
+    ZKAssign.createNodeOffline(server.getZooKeeper(), TEST_HRI, server.getServerName());
+    // Create the handler
+    OpenRegionHandler handler = new OpenRegionHandler(server, rsServices, TEST_HRI, TEST_HTD) {
+
+      @Override
+      boolean transitionZookeeperOfflineToOpening(String encodedName, int versionOfOfflineNode) {
+        return false;
+      }
+    };
+    rsServices.getRegionsInTransitionInRS().put(TEST_HRI.getEncodedNameAsBytes(), Boolean.TRUE);
+
+    handler.process();
+
+    RegionTransition rt = RegionTransition.parseFrom(ZKAssign.getData(server.getZooKeeper(),
+        TEST_HRI.getEncodedName()));
+    assertEquals(EventType.RS_ZK_REGION_FAILED_OPEN, rt.getEventType());
+  }
+
+  @Test
+  public void testTransitionToFailedOpenFromOffline() throws Exception {
+    Server server = new MockServer(HTU);
+    RegionServerServices rsServices = new MockRegionServerServices(server.getZooKeeper(),
+        server.getServerName());
+    // Create it OFFLINE, which is what it expects
+    ZKAssign.createNodeOffline(server.getZooKeeper(), TEST_HRI, server.getServerName());
+    // Create the handler
+    OpenRegionHandler handler = new OpenRegionHandler(server, rsServices, TEST_HRI, TEST_HTD) {
+
+      @Override
+      boolean transitionZookeeperOfflineToOpening(String encodedName, int versionOfOfflineNode) {
+        return false;
+      }
+    };
+    rsServices.getRegionsInTransitionInRS().put(TEST_HRI.getEncodedNameAsBytes(), Boolean.TRUE);
+
+    handler.process();
+
+    RegionTransition rt = RegionTransition.parseFrom(ZKAssign.getData(server.getZooKeeper(),
+        TEST_HRI.getEncodedName()));
+    assertEquals(EventType.RS_ZK_REGION_FAILED_OPEN, rt.getEventType());
+  }
 }
 

hbase-server/src/test/java/org/apache/hadoop/hbase/util/MockRegionServerServices.java

@@ -51,11 +51,17 @@ public class MockRegionServerServices implements RegionServerServices {
     new ConcurrentSkipListMap<byte[], Boolean>(Bytes.BYTES_COMPARATOR);
   private HFileSystem hfs = null;
   private ZooKeeperWatcher zkw = null;
+  private ServerName serverName = null;
 
-  public MockRegionServerServices(ZooKeeperWatcher zkw){
+  public MockRegionServerServices(ZooKeeperWatcher zkw) {
     this.zkw = zkw;
   }
   
+  public MockRegionServerServices(ZooKeeperWatcher zkw, ServerName serverName) {
+    this.zkw = zkw;
+    this.serverName = serverName;
+  }
+
   public MockRegionServerServices(){
     this(null);
   }
@@ -126,7 +132,7 @@ public class MockRegionServerServices implements RegionServerServices {
 
   @Override
   public ServerName getServerName() {
-    return null;
+    return this.serverName;
   }
 
   @Override