You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/09/18 17:28:36 UTC
[GitHub] [accumulo] tynyttie opened a new pull request #1710: WIP #873 Remove Sync and added StripedLock
tynyttie opened a new pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710
When tested, I did not notice any performance benefits.
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1710: WIP #873 Remove Sync and added StripedLock
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710#discussion_r519935616
##########
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:
Use of Beta can be dangerous as we have been previously burned by it. I did some work to remove uses of Beta code in Accumulo. See https://issues.apache.org/jira/browse/ACCUMULO-4702
I would listen to this important declaration:
> If your code is a library itself (i.e. it is used on the CLASSPATH of users outside your own control), you should not use beta API
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1710: WIP #873 Remove Sync and added StripedLock
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710#discussion_r491170209
##########
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:
Latest looks better. Thanks.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1710: WIP #873 Remove Sync and added StripedLock
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710#discussion_r518076732
##########
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:
Just as a note, it appears it has been in beta since 2012, v 13.0 (now v 30.0 is released). I don't expect it to leave beta anytime soon.
----------------------------------------------------------------
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
[GitHub] [accumulo] keith-turner commented on a change in pull request #1710: WIP #873 Remove Sync and added StripedLock
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710#discussion_r518464277
##########
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:
I would avoid Guava striped locks because its beta. I have used this pattern w/o Guava using an array of locks like :
https://github.com/apache/accumulo/blob/46ee51d0c6afe9910873f55653017b91c45a345a/core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/lru/SynchronousLoadingBlockCache.java#L38
----------------------------------------------------------------
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
[GitHub] [accumulo] milleruntime commented on a change in pull request #1710: WIP #873 Remove Sync and added StripedLock
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710#discussion_r519935616
##########
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:
Use of Beta can be dangerous as we have been burned previously. I did some work to remove uses of Beta code in Accumulo. See https://issues.apache.org/jira/browse/ACCUMULO-4702
I would listen to this important declaration:
> If your code is a library itself (i.e. it is used on the CLASSPATH of users outside your own control), you should not use beta API
----------------------------------------------------------------
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
[GitHub] [accumulo] tynyttie commented on a change in pull request #1710: WIP #873 Remove Sync and added StripedLock
Posted by GitBox <gi...@apache.org>.
tynyttie commented on a change in pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710#discussion_r491164394
##########
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:
Please check the latest commit. I sent the wrong version.
----------------------------------------------------------------
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
[GitHub] [accumulo] keith-turner commented on pull request #1710: WIP #873 Remove Sync and added StripedLock
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710#issuecomment-722740061
@tynyttie what type of test did you run? Was there concurrent bulk import operations running?
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1710: WIP #873 Remove Sync and added StripedLock
Posted by 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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1710: WIP #873 Remove Sync and added StripedLock
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1710:
URL: https://github.com/apache/accumulo/pull/1710#discussion_r518237595
##########
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 doesn't seem that long ago since we were using version 11 of Guava, because that's what Hadoop was using. Guava moves fast. The beta methods can change quickly and can quickly become incompatible with other libraries that require specific versions of Guava. I still think this is unsafe.
More importantly, if there's no observable performance benefit, it's not worth the risk.
----------------------------------------------------------------
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