You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:29:28 UTC
[GitHub] [hbase] bharathv opened a new pull request #3382: HBASE-25998: Redo synchronization in SyncFuture [DRAFT]
bharathv opened a new pull request #3382:
URL: https://github.com/apache/hbase/pull/3382
Currently uses coarse grained synchronized approach that seems to
create a lot of contention. This patch
- Use a reentrant lock instead of synchronized
- Condition variable based waiting rather than busy wait
- Removed synchronization for unnecessary fields..
--
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] [hbase] Apache9 commented on a change in pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on a change in pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#discussion_r652289075
##########
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) {
+ // First reset
+ return true;
+ }
+ doneLock.lock();
+ try {
+ return this.doneTxid != NOT_DONE;
+ } finally {
+ doneLock.unlock();
+ }
}
- synchronized Throwable getThrowable() {
- return this.throwable;
+ Throwable getThrowable() {
+ doneLock.lock();
+ try {
+ if (!isDone()) {
Review comment:
Better to just test this.doneTxid != NOT_DONE? Or introduce an unlocked isDone version may be called isDoneWithoutLock to call it in both isDone and getThrowable, so we do not need to lock in the isDone again 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] [hbase] Apache-HBase commented on pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#issuecomment-861128173
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 4s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 21s | master passed |
| +1 :green_heart: | compile | 1m 2s | master passed |
| +1 :green_heart: | shadedjars | 9m 4s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 4s | the patch passed |
| +1 :green_heart: | compile | 1m 1s | the patch passed |
| +1 :green_heart: | javac | 1m 1s | the patch passed |
| +1 :green_heart: | shadedjars | 9m 8s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 36s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 209m 55s | hbase-server in the patch passed. |
| | | 242m 48s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3382 |
| JIRA Issue | HBASE-25998 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 90dc0071aa3c 4.15.0-142-generic #146-Ubuntu SMP Tue Apr 13 01:11:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / ec31818574 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/testReport/ |
| Max. process+thread count | 3168 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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] [hbase] bharathv commented on pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
bharathv commented on pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#issuecomment-861041039
> In general, I do not think using ReentrantLock could have much difference on performance comparing to synchronized.
> Maybe the reason is because of now we create a ReentrantLock every time so there are some side effects such as biased locking?
Agree its not just synchronized vs j.u.concurrent. It looks like synchronized is a teeny bit slower compared to concurrent implementations under contention (based on benchmarks, [example](http://david-soroko.blogspot.com/2016/02/synchronized-vs-reentrantlock.html)) but in general there shouldn't be a substantial difference, especially with modern JVM versions.
I don't think it is biased locking because synchronized inherently supports biased locking (and interestingly it is [being removed](https://openjdk.java.net/jeps/374)) and that is not a differentiator.
I've looked at the assembly of the JIT-ed code for this function but it was not obvious to me (I'm not an expert at reading x86 assembly, so may I have missed something too). Just looking at the flame graphs with/without change I can see that there is less contention around locks due to a reasons like no busy wait, instead there is a CV wait (so there is no constant monitor contention in a loop until woken up, there is no contention around certain fields like txid, forcesync and probably some help from reentrant lock being faster than object monitors.
--
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] [hbase] Apache-HBase commented on pull request #3382: HBASE-25998: Redo synchronization in SyncFuture [DRAFT]
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#issuecomment-859716485
--
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] [hbase] Apache-HBase commented on pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#issuecomment-860946117
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 21s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 25s | master passed |
| +1 :green_heart: | compile | 1m 3s | master passed |
| +1 :green_heart: | shadedjars | 9m 46s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 53s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 9s | the patch passed |
| +1 :green_heart: | compile | 1m 24s | the patch passed |
| +1 :green_heart: | javac | 1m 24s | the patch passed |
| +1 :green_heart: | shadedjars | 11m 8s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 47s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 237m 32s | hbase-server in the patch passed. |
| | | 275m 31s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3382 |
| JIRA Issue | HBASE-25998 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 088014f95162 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / ba6995e083 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/2/testReport/ |
| Max. process+thread count | 2792 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/2/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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] [hbase] Apache-HBase commented on pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#issuecomment-860802563
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 8s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 21s | master passed |
| +1 :green_heart: | compile | 3m 23s | master passed |
| +1 :green_heart: | checkstyle | 1m 11s | master passed |
| +1 :green_heart: | spotbugs | 2m 13s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 3s | the patch passed |
| +1 :green_heart: | compile | 3m 22s | the patch passed |
| +1 :green_heart: | javac | 3m 22s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 10s | hbase-server: The patch generated 0 new + 11 unchanged - 1 fixed = 11 total (was 12) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 20m 3s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | spotbugs | 2m 23s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 13s | The patch does not generate ASF License warnings. |
| | | 51m 49s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3382 |
| JIRA Issue | HBASE-25998 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 3cea5fb03f01 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / ba6995e083 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Max. process+thread count | 86 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/2/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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] [hbase] Apache-HBase commented on pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#issuecomment-861059341
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 26s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 58s | master passed |
| +1 :green_heart: | compile | 3m 10s | master passed |
| +1 :green_heart: | checkstyle | 1m 3s | master passed |
| +1 :green_heart: | spotbugs | 2m 4s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 36s | the patch passed |
| +1 :green_heart: | compile | 3m 12s | the patch passed |
| +1 :green_heart: | javac | 3m 12s | the patch passed |
| -0 :warning: | checkstyle | 1m 2s | hbase-server: The patch generated 1 new + 11 unchanged - 1 fixed = 12 total (was 12) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 17m 58s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | spotbugs | 2m 13s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 15s | The patch does not generate ASF License warnings. |
| | | 46m 54s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3382 |
| JIRA Issue | HBASE-25998 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 0cf06f38b18a 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / ec31818574 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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] [hbase] Apache-HBase commented on pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#issuecomment-861106777
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 26s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 29s | master passed |
| +1 :green_heart: | compile | 1m 11s | master passed |
| +1 :green_heart: | shadedjars | 8m 6s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 43s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 11s | the patch passed |
| +1 :green_heart: | compile | 1m 14s | the patch passed |
| +1 :green_heart: | javac | 1m 14s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 13s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 41s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 147m 19s | hbase-server in the patch failed. |
| | | 178m 35s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3382 |
| JIRA Issue | HBASE-25998 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 56f173b3b4f0 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / ec31818574 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/testReport/ |
| Max. process+thread count | 4110 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/3/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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] [hbase] saintstack commented on pull request #3382: HBASE-25998: Redo synchronization in SyncFuture [DRAFT]
Posted by GitBox <gi...@apache.org>.
saintstack commented on pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#issuecomment-859998771
Looks good.
--
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] [hbase] Apache-HBase commented on pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#issuecomment-860888864
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 17s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 32s | master passed |
| +1 :green_heart: | compile | 1m 17s | master passed |
| +1 :green_heart: | shadedjars | 8m 14s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 43s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 16s | the patch passed |
| +1 :green_heart: | compile | 1m 16s | the patch passed |
| +1 :green_heart: | javac | 1m 16s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 42s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 40s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 144m 45s | hbase-server in the patch passed. |
| | | 178m 3s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3382 |
| JIRA Issue | HBASE-25998 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 7c01308e66c7 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / ba6995e083 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/2/testReport/ |
| Max. process+thread count | 4001 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3382/2/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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] [hbase] Apache9 commented on a change in pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [hbase] bharathv merged pull request #3382: HBASE-25998: Redo synchronization in SyncFuture
Posted by GitBox <gi...@apache.org>.
bharathv merged pull request #3382:
URL: https://github.com/apache/hbase/pull/3382
--
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] [hbase] bharathv commented on a change in pull request #3382: HBASE-25998: Redo synchronization in SyncFuture [DRAFT]
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #3382:
URL: https://github.com/apache/hbase/pull/3382#discussion_r650598109
##########
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:
Ack, 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:
users@infra.apache.org
[GitHub] [hbase] saintstack commented on a change in pull request #3382: HBASE-25998: Redo synchronization in SyncFuture [DRAFT]
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org