Apache9 commented on a change in pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#discussion_r652287183
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SyncFuture.java
##########
@@ -65,102 +74,128 @@
*/
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());
}
+ if (this.doneLock == null) {
+ this.doneLock = new ReentrantLock();
+ this.doneCondition = doneLock.newCondition();
+ }
this.doneTxid = NOT_DONE;
+ 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() {
+ long getTxid() {
return this.txid;
}
- synchronized boolean isForceSync() {
+ boolean isForceSync() {
return forceSync;
}
- synchronized SyncFuture setForceSync(boolean forceSync) {
- this.forceSync = forceSync;
- return this;
- }
-
/**
* @param txid the transaction id at which this future 'completed'.
* @param t Can be null. Set if we are 'completing' on error (and this 't'
is the error).
* @return True if we successfully marked this outstanding future as
completed/done. Returns false
* if this future is already 'done' when this method called.
*/
- synchronized boolean done(final long txid, final Throwable t) {
- if (isDone()) {
- return false;
- }
- this.throwable = t;
- if (txid < this.txid) {
- // Something badly wrong.
- if (throwable == null) {
- this.throwable =
- new IllegalStateException("done txid=" + txid + ", my txid=" +
this.txid);
+ boolean done(final long txid, final Throwable t) {
+ doneLock.lock();
+ try {
+ if (isDone()) {
+ return false;
}
+ this.throwable = t;
+ if (txid < this.txid) {
+ // Something badly wrong.
+ if (throwable == null) {
+ this.throwable =
+ new IllegalStateException("done txid=" + txid + ", my txid=" +
this.txid);
+ }
+ }
+ // Mark done.
+ this.doneTxid = txid;
+ doneCondition.signalAll();
+ return true;
+ } finally {
+ doneLock.unlock();
}
- // Mark done.
- this.doneTxid = txid;
- // Wake up waiting threads.
- notify();
- return true;
- }
-
- boolean cancel(boolean mayInterruptIfRunning) {
- throw new UnsupportedOperationException();
}
- synchronized long get(long timeoutNs) throws InterruptedException,
+ long get(long timeoutNs) throws InterruptedException,
ExecutionException, TimeoutIOException {
- final long done = System.nanoTime() + timeoutNs;
- while (!isDone()) {
- wait(1000);
- if (System.nanoTime() >= done) {
- throw new TimeoutIOException(
- "Failed to get sync result after " +
TimeUnit.NANOSECONDS.toMillis(timeoutNs)
- + " ms for txid=" + this.txid + ", WAL system stuck?");
+ doneLock.lock();
+ try {
+ while (!isDone()) {
+ if (!doneCondition.await(timeoutNs, TimeUnit.NANOSECONDS)) {
+ throw new TimeoutIOException("Failed to get sync result after "
+ + TimeUnit.NANOSECONDS.toMillis(timeoutNs) + " ms for txid=" +
this.txid
+ + ", WAL system stuck?");
+ }
}
+ if (this.throwable != null) {
+ throw new ExecutionException(this.throwable);
+ }
+ return this.doneTxid;
+ } finally {
+ doneLock.unlock();
}
- if (this.throwable != null) {
- throw new ExecutionException(this.throwable);
- }
- return this.doneTxid;
- }
-
- synchronized boolean isDone() {
- return this.doneTxid != NOT_DONE;
}
- synchronized boolean isThrowable() {
- return isDone() && getThrowable() != null;
+ boolean isDone() {
+ if (doneLock == null) {
Review comment:
This is a bit confusing... What if later a developer call this method
before calling reset? Since now we will not create the lock and condition
everytime in reset, let's just create it ion the constructor? So we do not need
to test whether it is null any more.
--
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]
