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 2020/12/25 10:07:40 UTC
[GitHub] [hbase] nyl3532016 opened a new pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
nyl3532016 opened a new pull request #2814:
URL: https://github.com/apache/hbase/pull/2814
----------------------------------------------------------------
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 #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#discussion_r548889401
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -36,13 +36,13 @@
*/
@InterfaceAudience.Private
public class MultiVersionConcurrencyControl {
- private static final Logger LOG = LoggerFactory.getLogger(MultiVersionConcurrencyControl.class);
private static final long READPOINT_ADVANCE_WAIT_TIME = 10L;
final String regionName;
final AtomicLong readPoint = new AtomicLong(0);
final AtomicLong writePoint = new AtomicLong(0);
- private final Object readWaiters = new Object();
+
+ private final ThreadLocal<WriteEntry> cachedWriteEntries;
Review comment:
Is it always safe to use ThreadLocal here? What if users just use ASYNC_WAL or even NO_WAL and return without waiting for MVCC to move？
----------------------------------------------------------------
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] nyl3532016 commented on a change in pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
nyl3532016 commented on a change in pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#discussion_r549594528
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -140,7 +146,7 @@ public WriteEntry begin() {
public WriteEntry begin(Runnable action) {
synchronized (writeQueue) {
long nextWriteNumber = writePoint.incrementAndGet();
- WriteEntry e = new WriteEntry(nextWriteNumber);
+ WriteEntry e = cachedWriteEntries.get().setWriteNumber(nextWriteNumber).markCompleted(false);
Review comment:
after change `WriteEntry` to threadlocal.
It is ok to this scene in one thread: `mvcc1(begin)---mvcc1(wait complete)---mvcc2(begin)---mvcc2(wait complete)`
but is bad to the scene in one thread: `mvcc1(begin)---mvcc2(begin)---mvcc1(wait complete)---mvcc2(wait complete)`
I think our usage of `WriteEntry` is the former
----------------------------------------------------------------
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] nyl3532016 commented on pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
nyl3532016 commented on pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#issuecomment-751971855
> Have to be careful changing this code. It is hard to get it right.
>
> The narrow notify in place of the notifyAll is nice but I see lots of new synchronize blocks in this PR when we check for state change; do these new synchronizes undo the advantage of the notify on WriteEntry only? Can you demonstrate improved throughput?
Let me test via ycsb to see wether it can improve throughput.
----------------------------------------------------------------
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] nyl3532016 commented on a change in pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
nyl3532016 commented on a change in pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#discussion_r549242127
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -2754,10 +2754,10 @@ protected PrepareFlushResult internalPrepareFlushCache(WAL wal, long myseqid,
if (wal != null) {
writeEntry = mvcc.begin();
long flushOpSeqId = writeEntry.getWriteNumber();
+ mvcc.completeAndWait(writeEntry);
FlushResultImpl flushResult =
new FlushResultImpl(FlushResult.Result.CANNOT_FLUSH_MEMSTORE_EMPTY, flushOpSeqId,
"Nothing to flush", writeFlushRequestMarkerToWAL(wal, writeFlushWalMarker));
- mvcc.completeAndWait(writeEntry);
Review comment:
if `writeEntry` be threadLocal, the logic here is wrong.
`writeFlushRequestMarkerToWAL` contains mvcc begin and complete
----------------------------------------------------------------
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 #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#discussion_r549462374
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -140,7 +146,7 @@ public WriteEntry begin() {
public WriteEntry begin(Runnable action) {
synchronized (writeQueue) {
long nextWriteNumber = writePoint.incrementAndGet();
- WriteEntry e = new WriteEntry(nextWriteNumber);
+ WriteEntry e = cachedWriteEntries.get().setWriteNumber(nextWriteNumber).markCompleted(false);
Review comment:
Should we have protection here in case we overwrite an on-going WriteEntry? One that is not complete?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -36,13 +36,13 @@
*/
@InterfaceAudience.Private
public class MultiVersionConcurrencyControl {
- private static final Logger LOG = LoggerFactory.getLogger(MultiVersionConcurrencyControl.class);
private static final long READPOINT_ADVANCE_WAIT_TIME = 10L;
final String regionName;
final AtomicLong readPoint = new AtomicLong(0);
final AtomicLong writePoint = new AtomicLong(0);
- private final Object readWaiters = new Object();
+
+ private final ThreadLocal<WriteEntry> cachedWriteEntries;
Review comment:
Needs comment explaining lifeccycle of cachedWriteEntries. Why is it plural? Should it be cachedWriteEntry? Why not declare and initialize here rather then declare and then initialize in the constructor?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -272,33 +252,59 @@ public long getWritePoint() {
*/
@InterfaceAudience.Private
public static class WriteEntry {
- private final long writeNumber;
+ private long writeNumber = 0L;
private boolean completed = false;
+ private static final Logger LOG = LoggerFactory.getLogger(WriteEntry.class);
- WriteEntry(long writeNumber) {
- this.writeNumber = writeNumber;
+ synchronized WriteEntry markCompleted(boolean completed) {
Review comment:
Do we need to pass a boolean here? We are always setting complete to true? We ever set it to false? This methods should take no parameters?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -272,33 +252,59 @@ public long getWritePoint() {
*/
@InterfaceAudience.Private
public static class WriteEntry {
- private final long writeNumber;
+ private long writeNumber = 0L;
Review comment:
Because it only written by a single, thread this is safe to do.... it doesn't need to be volatile.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -36,13 +36,13 @@
*/
@InterfaceAudience.Private
public class MultiVersionConcurrencyControl {
- private static final Logger LOG = LoggerFactory.getLogger(MultiVersionConcurrencyControl.class);
private static final long READPOINT_ADVANCE_WAIT_TIME = 10L;
final String regionName;
final AtomicLong readPoint = new AtomicLong(0);
final AtomicLong writePoint = new AtomicLong(0);
- private final Object readWaiters = new Object();
+
+ private final ThreadLocal<WriteEntry> cachedWriteEntries;
Review comment:
I like the idea of saving on allocation of a WriteEntry.
The @Apache9 questions are good ones.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -185,7 +191,7 @@ public void completeAndWait(WriteEntry e) {
*/
public boolean complete(WriteEntry writeEntry) {
synchronized (writeQueue) {
- writeEntry.markCompleted();
+ writeEntry.markCompleted(true);
Review comment:
Why pass the boolean?
----------------------------------------------------------------
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] nyl3532016 commented on a change in pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
nyl3532016 commented on a change in pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#discussion_r551230988
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -185,7 +191,7 @@ public void completeAndWait(WriteEntry e) {
*/
public boolean complete(WriteEntry writeEntry) {
synchronized (writeQueue) {
- writeEntry.markCompleted();
+ writeEntry.markCompleted(true);
Review comment:
Yes,will use it in the place:
`WriteEntry e = cachedWriteEntries.get().setWriteNumber(nextWriteNumber).markCompleted(false);`
Let me use `setCompleted` instead
----------------------------------------------------------------
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 #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#issuecomment-751245670
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 28s | 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 | 3m 49s | master passed |
| +1 :green_heart: | compile | 0m 59s | master passed |
| +1 :green_heart: | shadedjars | 6m 35s | 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 | 3m 31s | 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 | 7m 38s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 38s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 141m 43s | hbase-server in the patch failed. |
| | | 169m 17s | |
| 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-2814/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2814 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 83373bbb8c1c 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 0f868da05d |
| Default Java | AdoptOpenJDK-1.8.0_232-b09 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2814/1/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2814/1/testReport/ |
| Max. process+thread count | 4195 (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-2814/1/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 #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#issuecomment-751244868
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 37s | 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 31s | master passed |
| +1 :green_heart: | compile | 1m 8s | master passed |
| +1 :green_heart: | shadedjars | 6m 55s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 44s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 7s | the patch passed |
| +1 :green_heart: | compile | 1m 12s | the patch passed |
| +1 :green_heart: | javac | 1m 12s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 47s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 40s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 132m 33s | hbase-server in the patch failed. |
| | | 161m 26s | |
| 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-2814/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2814 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux c4162ce6c811 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 / 0f868da05d |
| Default Java | AdoptOpenJDK-11.0.6+10 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2814/1/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-2814/1/testReport/ |
| Max. process+thread count | 4367 (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-2814/1/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] nyl3532016 commented on a change in pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
nyl3532016 commented on a change in pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#discussion_r549203755
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -36,13 +36,13 @@
*/
@InterfaceAudience.Private
public class MultiVersionConcurrencyControl {
- private static final Logger LOG = LoggerFactory.getLogger(MultiVersionConcurrencyControl.class);
private static final long READPOINT_ADVANCE_WAIT_TIME = 10L;
final String regionName;
final AtomicLong readPoint = new AtomicLong(0);
final AtomicLong writePoint = new AtomicLong(0);
- private final Object readWaiters = new Object();
+
+ private final ThreadLocal<WriteEntry> cachedWriteEntries;
Review comment:
Let me check the multi-thread safety more carefully.
User will wait for MVCC to move when use `ASYNC_WAL` or even `NO_WAL`
`ASYNC_WAL` only effect phase of WAL sync which contained in the period from `WriteEntry` generate to `MVCC` move, `NO_WAL` will generate `WriteEntry` while write data to memstore
----------------------------------------------------------------
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] nyl3532016 commented on a change in pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
nyl3532016 commented on a change in pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#discussion_r549587869
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -185,7 +191,7 @@ public void completeAndWait(WriteEntry e) {
*/
public boolean complete(WriteEntry writeEntry) {
synchronized (writeQueue) {
- writeEntry.markCompleted();
+ writeEntry.markCompleted(true);
Review comment:
maybe should change the function name from `markCompleted` to `setCompleted`
----------------------------------------------------------------
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] nyl3532016 commented on pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
nyl3532016 commented on pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#issuecomment-757652851
[50M-performance.pdf](https://github.com/apache/hbase/files/5794265/50M-performance.pdf)
@Apache9 @saintstack, I test the performance, not improvement but even a little degradation
----------------------------------------------------------------
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 #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#issuecomment-751623216
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | 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: | checkstyle | 1m 6s | master passed |
| +1 :green_heart: | spotbugs | 2m 6s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 26s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 7s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 18m 53s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| -1 :x: | spotbugs | 3m 2s | hbase-server generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 16s | The patch does not generate ASF License warnings. |
| | | 44m 1s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:hbase-server |
| | Naked notify in org.apache.hadoop.hbase.regionserver.MultiVersionConcurrencyControl$WriteEntry.done() At MultiVersionConcurrencyControl.java:At MultiVersionConcurrencyControl.java:[line 277] |
| 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-2814/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2814 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux 19c4da19d90d 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 / 0f868da05d |
| spotbugs | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2814/2/artifact/yetus-general-check/output/new-spotbugs-hbase-server.html |
| Max. process+thread count | 94 (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-2814/2/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=3.1.12 |
| 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 #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#issuecomment-751660113
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 4m 18s | 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 | 3m 49s | master passed |
| +1 :green_heart: | compile | 0m 58s | master passed |
| +1 :green_heart: | shadedjars | 6m 37s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 40s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 32s | the patch passed |
| +1 :green_heart: | compile | 0m 58s | the patch passed |
| +1 :green_heart: | javac | 0m 58s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 33s | 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 | 139m 57s | hbase-server in the patch passed. |
| | | 170m 2s | |
| 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-2814/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2814 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux b3b63eeaf93e 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 0f868da05d |
| Default Java | AdoptOpenJDK-1.8.0_232-b09 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2814/2/testReport/ |
| Max. process+thread count | 3973 (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-2814/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 #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#issuecomment-751657211
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | 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 36s | master passed |
| +1 :green_heart: | compile | 1m 10s | master passed |
| +1 :green_heart: | shadedjars | 6m 53s | 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 10s | the patch passed |
| +1 :green_heart: | javac | 1m 10s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 48s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 41s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 131m 20s | hbase-server in the patch passed. |
| | | 160m 21s | |
| 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-2814/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2814 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux c8093edf36c4 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 / 0f868da05d |
| Default Java | AdoptOpenJDK-11.0.6+10 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2814/2/testReport/ |
| Max. process+thread count | 4115 (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-2814/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] saintstack commented on a change in pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#discussion_r550912921
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -185,7 +191,7 @@ public void completeAndWait(WriteEntry e) {
*/
public boolean complete(WriteEntry writeEntry) {
synchronized (writeQueue) {
- writeEntry.markCompleted();
+ writeEntry.markCompleted(true);
Review comment:
Do we ever pass false? I don't see it (perhaps we do). If we don't, then markCompleted seems fine (without a param). setCompleted makes sense if a param is passed.
----------------------------------------------------------------
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] nyl3532016 commented on a change in pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
nyl3532016 commented on a change in pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#discussion_r549587198
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MultiVersionConcurrencyControl.java
##########
@@ -272,33 +252,59 @@ public long getWritePoint() {
*/
@InterfaceAudience.Private
public static class WriteEntry {
- private final long writeNumber;
+ private long writeNumber = 0L;
private boolean completed = false;
+ private static final Logger LOG = LoggerFactory.getLogger(WriteEntry.class);
- WriteEntry(long writeNumber) {
- this.writeNumber = writeNumber;
+ synchronized WriteEntry markCompleted(boolean completed) {
Review comment:
when reuse the thread local `WriteEntry`, we must set it incomplete
----------------------------------------------------------------
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 #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2814:
URL: https://github.com/apache/hbase/pull/2814#issuecomment-751231628
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 36s | 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 54s | master passed |
| +1 :green_heart: | checkstyle | 1m 5s | master passed |
| +1 :green_heart: | spotbugs | 2m 5s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 24s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 2s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 18m 23s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| -1 :x: | spotbugs | 2m 57s | hbase-server generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 16s | The patch does not generate ASF License warnings. |
| | | 43m 18s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:hbase-server |
| | Naked notify in org.apache.hadoop.hbase.regionserver.MultiVersionConcurrencyControl$WriteEntry.done() At MultiVersionConcurrencyControl.java:At MultiVersionConcurrencyControl.java:[line 277] |
| 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-2814/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2814 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux b5d6fc9287fc 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 / 0f868da05d |
| spotbugs | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2814/1/artifact/yetus-general-check/output/new-spotbugs-hbase-server.html |
| Max. process+thread count | 94 (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-2814/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=3.1.12 |
| 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] nyl3532016 closed pull request #2814: HBASE-25442 Refactor MultiVersionConcurrencyControl: replace readWaiters.notifyAll() with WriteEntry.notify()
Posted by GitBox <gi...@apache.org>.
nyl3532016 closed pull request #2814:
URL: https://github.com/apache/hbase/pull/2814
----------------------------------------------------------------
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