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:
us...@infra.apache.org
