Apache9 commented on a change in pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#discussion_r650926329
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AsyncFSWAL.java
##########
@@ -132,7 +131,7 @@
private static final Logger LOG = LoggerFactory.getLogger(AsyncFSWAL.class);
private static final Comparator<SyncFuture> SEQ_COMPARATOR = (o1, o2) -> {
- int c = Long.compare(o1.getTxid(), o2.getTxid());
+ int c = Long.compare(o1.getTxId(), o2.getTxId());
Review comment:
And maybe for JDK8, we could use
Comparator.comparingLong(SyncFuture::getTxid).thenComparingInt(System::identityHashCode);
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AsyncFSWAL.java
##########
@@ -132,7 +131,7 @@
private static final Logger LOG = LoggerFactory.getLogger(AsyncFSWAL.class);
private static final Comparator<SyncFuture> SEQ_COMPARATOR = (o1, o2) -> {
- int c = Long.compare(o1.getTxid(), o2.getTxid());
+ int c = Long.compare(o1.getTxId(), o2.getTxId());
Review comment:
Why this change? I think in most places we just use txid instead of
txId...
##########
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();
Review comment:
The lock is also reset everytime?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SyncFuture.java
##########
@@ -44,15 +45,23 @@
*/
@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.
+ * Lock protecting the thread-safe fields.
+ */
+ ReentrantLock doneLock;
+
+ /**
+ * Condition to wait on for client threads.
+ */
+ Condition doneCondition;
Review comment:
Ditto.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SyncFuture.java
##########
@@ -44,15 +45,23 @@
*/
@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.
+ * Lock protecting the thread-safe fields.
+ */
+ ReentrantLock doneLock;
Review comment:
Could this be private?
--
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:
us...@infra.apache.org
