[GitHub] [accumulo] ctubbsii commented on a change in pull request #1710: WIP #873 Remove Sync and added StripedLock

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710#discussion_r491132489



##########
File path: server/base/src/main/java/org/apache/accumulo/server/zookeeper/TransactionWatcher.java
##########
@@ -128,44 +131,59 @@ public boolean transactionComplete(String type, long tid) throws Exception {
    * that task is not run and a debug message is logged indicating the task was ignored.
    */
   public void runQuietly(String ztxBulk, long tid, Runnable task) {
-    synchronized (counts) {
-      try {
-        if (!arbitrator.transactionAlive(ztxBulk, tid)) {
-          log.debug("Transaction " + tid + " of type " + ztxBulk + " is no longer active.");
-          return;
-        }
-      } catch (Exception e) {
-        log.warn("Unable to check if transaction " + tid + " of type " + ztxBulk + " is alive ", e);
+
+    Lock l = stripedLocks.get(tid);
+    l.lock();
+    try {
+      if (!arbitrator.transactionAlive(ztxBulk, tid)) {
+        log.debug("Transaction " + tid + " of type " + ztxBulk + " is no longer active.");
         return;
       }
-      increment(tid);
+    } catch (Exception e) {
+      log.warn("Unable to check if transaction " + tid + " of type " + ztxBulk + " is alive ", e);
+      return;
     }
+    increment(tid);
     try {
       task.run();
     } finally {
       decrement(tid);
+      l.lock();
     }
+

Review comment:
       The locking in this method doesn't look right. There are two locks, no unlocks, and the locks are never released in a finally block.

##########
File path: server/base/src/main/java/org/apache/accumulo/server/zookeeper/TransactionWatcher.java
##########
@@ -25,7 +25,9 @@
 import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.Lock;
 
+import com.google.common.util.concurrent.Striped;

Review comment:
       It looks like this is `@Beta`.

##########
File path: server/base/src/main/java/org/apache/accumulo/server/zookeeper/TransactionWatcher.java
##########
@@ -128,44 +131,59 @@ public boolean transactionComplete(String type, long tid) throws Exception {
    * that task is not run and a debug message is logged indicating the task was ignored.
    */
   public void runQuietly(String ztxBulk, long tid, Runnable task) {
-    synchronized (counts) {
-      try {
-        if (!arbitrator.transactionAlive(ztxBulk, tid)) {
-          log.debug("Transaction " + tid + " of type " + ztxBulk + " is no longer active.");
-          return;
-        }
-      } catch (Exception e) {
-        log.warn("Unable to check if transaction " + tid + " of type " + ztxBulk + " is alive ", e);
+
+    Lock l = stripedLocks.get(tid);
+    l.lock();
+    try {
+      if (!arbitrator.transactionAlive(ztxBulk, tid)) {
+        log.debug("Transaction " + tid + " of type " + ztxBulk + " is no longer active.");
         return;
       }
-      increment(tid);
+    } catch (Exception e) {
+      log.warn("Unable to check if transaction " + tid + " of type " + ztxBulk + " is alive ", e);
+      return;
     }
+    increment(tid);
     try {
       task.run();
     } finally {
       decrement(tid);
+      l.lock();
     }
+
   }
 
   public <T> T run(String ztxBulk, long tid, Callable<T> callable) throws Exception {
-    synchronized (counts) {
-      if (!arbitrator.transactionAlive(ztxBulk, tid)) {
-        throw new Exception("Transaction " + tid + " of type " + ztxBulk + " is no longer active");
-      }
-      increment(tid);
+    Lock l = stripedLocks.get(tid);
+    l.lock();
+
+    if (!arbitrator.transactionAlive(ztxBulk, tid)) {
+      throw new Exception("Transaction " + tid + " of type " + ztxBulk + " is no longer active");
     }
+    increment(tid);

Review comment:
       Shouldn't this code be either before the lock, or inside the try, so the lock is released in the finally block if there is an exception thrown in here?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org