HBASE-16233 Procedure V2: Support acquire/release shared table lock concurrently (Stephen Yuan Jiang)

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

@@ -794,21 +794,28 @@ public class MasterProcedureScheduler implements ProcedureRunnableSet {
 
     private synchronized boolean tryZkSharedLock(final TableLockManager lockManager,
         final String purpose) {
-      // Take zk-read-lock
+      // Since we only have one lock resource.  We should only acquire zk lock if the znode
-      TableName tableName = getKey();
+      // does not exist.
-      tableLock = lockManager.readLock(tableName, purpose);
+      //
-      try {
+      if (isSingleSharedLock()) {
-        tableLock.acquire();
+        // Take zk-read-lock
-      } catch (IOException e) {
+        TableName tableName = getKey();
-        LOG.error("failed acquire read lock on " + tableName, e);
+        tableLock = lockManager.readLock(tableName, purpose);
-        tableLock = null;
+        try {
-        return false;
+          tableLock.acquire();
+        } catch (IOException e) {
+          LOG.error("failed acquire read lock on " + tableName, e);
+          tableLock = null;
+          return false;
+        }
       }
       return true;
     }
 
     private synchronized void releaseZkSharedLock(final TableLockManager lockManager) {
-      releaseTableLock(lockManager, isSingleSharedLock());
+      if (isSingleSharedLock()) {
+        releaseTableLock(lockManager, true);
+      }
     }
 
     private synchronized boolean tryZkExclusiveLock(final TableLockManager lockManager,
@@ -969,16 +976,17 @@ public class MasterProcedureScheduler implements ProcedureRunnableSet {
       return null;
     }
 
-    schedLock.unlock();
+    // TODO: Zk lock is expensive and it would be perf bottleneck.  Long term solution is
-
+    // to remove it.
-    // Zk lock is expensive...
     if (!queue.tryZkSharedLock(lockManager, procedure.toString())) {
-      schedLock.lock();
       queue.releaseSharedLock();
       queue.getNamespaceQueue().releaseSharedLock();
       schedLock.unlock();
       return null;
     }
+
+    schedLock.unlock();
+
     return queue;
   }
 
@@ -990,10 +998,10 @@ public class MasterProcedureScheduler implements ProcedureRunnableSet {
   public void releaseTableSharedLock(final Procedure procedure, final TableName table) {
     final TableQueue queue = getTableQueueWithLock(table);
 
+    schedLock.lock();
     // Zk lock is expensive...
     queue.releaseZkSharedLock(lockManager);
 
-    schedLock.lock();
     queue.releaseSharedLock();
     queue.getNamespaceQueue().releaseSharedLock();
     schedLock.unlock();

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

@@ -18,6 +18,7 @@
 
 package org.apache.hadoop.hbase.master.procedure;
 
+import java.io.File;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashSet;
@@ -28,16 +29,19 @@ import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
 import org.apache.hadoop.hbase.HRegionInfo;
+import org.apache.hadoop.hbase.ServerName;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.master.TableLockManager;
 import org.apache.hadoop.hbase.master.procedure.MasterProcedureScheduler.ProcedureEvent;
 import org.apache.hadoop.hbase.procedure2.Procedure;
 import org.apache.hadoop.hbase.procedure2.ProcedureTestingUtility.TestProcedure;
-import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
 import org.apache.hadoop.hbase.testclassification.MasterTests;
 import org.apache.hadoop.hbase.util.Bytes;
-
+import org.apache.hadoop.hbase.zookeeper.MiniZooKeeperCluster;
+import org.apache.hadoop.hbase.zookeeper.ZooKeeperWatcher;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -47,7 +51,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
-@Category({MasterTests.class, SmallTests.class})
+@Category({MasterTests.class, MediumTests.class})
 public class TestMasterProcedureScheduler {
   private static final Log LOG = LogFactory.getLog(TestMasterProcedureScheduler.class);
 
@@ -349,6 +353,38 @@ public class TestMasterProcedureScheduler {
     assertEquals(4, procId);
   }
 
+  @Test
+  public void testSharedZkLock() throws Exception {
+    final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
+    final String dir = TEST_UTIL.getDataTestDir("TestSharedZkLock").toString();
+    MiniZooKeeperCluster zkCluster = new MiniZooKeeperCluster(conf);
+    int zkPort = zkCluster.startup(new File(dir));
+
+    try {
+      conf.set("hbase.zookeeper.quorum", "localhost:" + zkPort);
+
+      ZooKeeperWatcher zkw = new ZooKeeperWatcher(conf, "testSchedWithZkLock", null, false);
+      ServerName mockName = ServerName.valueOf("localhost", 60000, 1);
+      MasterProcedureScheduler procQueue = new MasterProcedureScheduler(
+        conf,
+        TableLockManager.createTableLockManager(conf, zkw, mockName));
+
+      final TableName tableName = TableName.valueOf("testtb");
+      TestTableProcedure procA =
+          new TestTableProcedure(1, tableName, TableProcedureInterface.TableOperationType.READ);
+      TestTableProcedure procB =
+          new TestTableProcedure(2, tableName, TableProcedureInterface.TableOperationType.READ);
+
+      assertTrue(procQueue.tryAcquireTableSharedLock(procA, tableName));
+      assertTrue(procQueue.tryAcquireTableSharedLock(procB, tableName));
+
+      procQueue.releaseTableSharedLock(procA, tableName);
+      procQueue.releaseTableSharedLock(procB, tableName);
+    } finally {
+      zkCluster.shutdown();
+    }
+  }
+
   @Test
   public void testVerifyRegionLocks() throws Exception {
     final TableName tableName = TableName.valueOf("testtb");