saintstack commented on a change in pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#discussion_r650346060
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SyncFuture.java
##########
@@ -44,19 +45,22 @@
*/
@InterfaceAudience.Private
class SyncFuture {
- // Implementation notes: I tried using a cyclicbarrier in here for handler
and sync threads
- // to coordinate on but it did not give any obvious advantage and some
issues with order in which
- // events happen.
+
private static final long NOT_DONE = -1L;
+ private Thread t;
- /**
- * The transaction id of this operation, monotonically increases.
- */
- private long txid;
+ // Lock protecting the thread safe fields
+ ReentrantLock doneLock;
+ // Condition to wait on for client threads.
+ Condition isDone;
Review comment:
And name it something else because isDone method already?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SyncFuture.java
##########
@@ -44,19 +45,22 @@
*/
@InterfaceAudience.Private
class SyncFuture {
- // Implementation notes: I tried using a cyclicbarrier in here for handler
and sync threads
- // to coordinate on but it did not give any obvious advantage and some
issues with order in which
- // events happen.
+
private static final long NOT_DONE = -1L;
+ private Thread t;
- /**
- * The transaction id of this operation, monotonically increases.
- */
- private long txid;
+ // Lock protecting the thread safe fields
+ ReentrantLock doneLock;
+ // Condition to wait on for client threads.
+ Condition isDone;
Review comment:
doneCondition?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SyncFuture.java
##########
@@ -44,19 +45,22 @@
*/
@InterfaceAudience.Private
class SyncFuture {
- // Implementation notes: I tried using a cyclicbarrier in here for handler
and sync threads
- // to coordinate on but it did not give any obvious advantage and some
issues with order in which
- // events happen.
+
private static final long NOT_DONE = -1L;
+ private Thread t;
- /**
- * The transaction id of this operation, monotonically increases.
- */
- private long txid;
+ // Lock protecting the thread safe fields
+ ReentrantLock doneLock;
+ // Condition to wait on for client threads.
+ Condition isDone;
Review comment:
nit: isDone is name of a method, not of a data member.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SyncFuture.java
##########
@@ -65,102 +74,126 @@
*/
private Throwable throwable;
- private Thread t;
+
+ /*
+ * Fields below are created once at reset() and accessed without any lock.
Should be ok as they
+ * are immutable for this instance of sync future until it is reset.
Review comment:
Is it worth it doing all this handling just so we reuse SyncFuture
instances? Maybe a later experiment would be trying to create a SyncFuture
every time? Just a thought.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SyncFuture.java
##########
@@ -65,102 +74,126 @@
*/
private Throwable throwable;
- private Thread t;
+
+ /*
+ * Fields below are created once at reset() and accessed without any lock.
Should be ok as they
+ * are immutable for this instance of sync future until it is reset.
+ */
+
+ /**
+ * The transaction id of this operation, monotonically increases.
+ */
+ private long txId;
private boolean forceSync;
/**
* Call this method to clear old usage and get it ready for new deploy.
+ *
* @param txid the new transaction id
* @return this
*/
- synchronized SyncFuture reset(long txid) {
+ SyncFuture reset(long txid, boolean forceSync) {
if (t != null && t != Thread.currentThread()) {
throw new IllegalStateException();
}
t = Thread.currentThread();
if (!isDone()) {
throw new IllegalStateException("" + txid + " " +
Thread.currentThread());
}
+ this.doneLock = new ReentrantLock();
+ this.doneCondition = doneLock.newCondition();
this.doneTxid = NOT_DONE;
- this.txid = txid;
+ this.forceSync = forceSync;
+ this.txId = txid;
this.throwable = null;
return this;
}
@Override
- public synchronized String toString() {
- return "done=" + isDone() + ", txid=" + this.txid;
+ public String toString() {
+ return "done=" + isDone() + ", txId=" + this.txId;
}
- synchronized long getTxid() {
- return this.txid;
+ long getTxId() {
+ return this.txId;
}
- synchronized boolean isForceSync() {
+ boolean isForceSync() {
return forceSync;
Review comment:
This should be covered by the lock? The comment at head of the class
says this data member should be covered by the lock? ("all below")
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AsyncFSWAL.java
##########
@@ -363,7 +362,8 @@ private void syncCompleted(AsyncWriter writer, long
processedTxid, long startTim
// sync futures then just use the default one.
private boolean isHsync(long beginTxid, long endTxid) {
Review comment:
Just to note that above the methods are changed from getTxid to getTxId
but then here we have beginTxid... super nit... but you might want them to be
consistent? Just leave the getTxid?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SyncFuture.java
##########
@@ -1,5 +1,5 @@
/**
- * Licensed to the Apache Software Foundation (ASF) under one
+ *Licensed to the Apache Software Foundation (ASF) under one
Review comment:
super super super nit...^ Put space back
--
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:
[email protected]
