bharathv commented on a change in pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#discussion_r651137633
##########
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:
Done.
##########
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:
Ya, the whole SyncFuture caching seems unnecessary, I was discussing
this with my colleagues at work. cc: @apurtell @d-c-manning
I don't have the full historical context here but if I were to guess I think
it is because we don't want GC to (over) work with millions of these small sync
future objects since they are created once per mutation. May be the latest
versions of Java garbage collectors don't need these kind of optimizations,
agree that this needs more performance analysis. Let me create a jira to perf
analyze the removal of this.
##########
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:
The whole Txid change mess was unintentional, undone.
##########
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:
Txid change was unintentional, undone.
Comparator change: Done.
##########
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:
There is also this comment..
```
/*
* 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.
```
##########
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:
I was just being extra cautions, just in case there are any dangling
locks and such from previous invocation but thinking about it again, that is
probably not needed.
##########
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:
Done.
##########
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:
Done.
--
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]
