anoopsjohn commented on a change in pull request #2021:
URL: https://github.com/apache/hbase/pull/2021#discussion_r453212134
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##########
@@ -249,4 +248,35 @@ public void close() {
running = false;
interrupt();
}
+
+ /**
+ * Independently control the roll of each wal. When use multiwal,
+ * can avoid all wal roll together. see HBASE-24665 for detail
+ */
+ protected class RollController {
+ boolean isRequestRoll;
+ long lastRollTime;
+
+ RollController() {
+ this.isRequestRoll = false;
+ this.lastRollTime = System.currentTimeMillis();
+ }
+
+ void requestRoll() {
+ this.isRequestRoll = true;
+ }
+
+ void finishRoll() {
Review comment:
This name is bit confusing. This is not called once roll is finished. We
can just call this resetStatus()? Give proper comment that this resets rollReq
status as well as lastRollTime. We can pass the ts as param 'lastRollTime' so
that this is clear.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##########
@@ -249,4 +248,35 @@ public void close() {
running = false;
interrupt();
}
+
+ /**
+ * Independently control the roll of each wal. When use multiwal,
+ * can avoid all wal roll together. see HBASE-24665 for detail
+ */
+ protected class RollController {
+ boolean isRequestRoll;
+ long lastRollTime;
+
+ RollController() {
+ this.isRequestRoll = false;
+ this.lastRollTime = System.currentTimeMillis();
+ }
+
+ void requestRoll() {
+ this.isRequestRoll = true;
+ }
+
+ void finishRoll() {
+ this.isRequestRoll = false;
+ this.lastRollTime = System.currentTimeMillis();
+ }
+
+ public boolean isRequestRoll() {
Review comment:
isRollRequested() can be the better name?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##########
@@ -249,4 +248,35 @@ public void close() {
running = false;
interrupt();
}
+
+ /**
+ * Independently control the roll of each wal. When use multiwal,
+ * can avoid all wal roll together. see HBASE-24665 for detail
+ */
+ protected class RollController {
+ boolean isRequestRoll;
+ long lastRollTime;
+
+ RollController() {
+ this.isRequestRoll = false;
+ this.lastRollTime = System.currentTimeMillis();
+ }
+
+ void requestRoll() {
+ this.isRequestRoll = true;
+ }
+
+ void finishRoll() {
+ this.isRequestRoll = false;
+ this.lastRollTime = System.currentTimeMillis();
+ }
+
+ public boolean isRequestRoll() {
+ return isRequestRoll;
+ }
+
+ boolean isPeriodRoll(long now) {
Review comment:
A better name ? needsPeriodicRoll ?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##########
@@ -174,18 +170,21 @@ public void run() {
}
}
try {
- this.lastRollTime = System.currentTimeMillis();
- for (Iterator<Entry<WAL, Boolean>> iter =
walNeedsRoll.entrySet().iterator(); iter
- .hasNext();) {
- Entry<WAL, Boolean> entry = iter.next();
+ for (Iterator<Entry<WAL, RollController>> iter =
walNeedsRoll.entrySet().iterator();
+ iter.hasNext();) {
+ Entry<WAL, RollController> entry = iter.next();
+ RollController controller = entry.getValue();
+ if (!controller.isRequestRoll && !controller.isPeriodRoll(now)) {
+ continue;
+ }
WAL wal = entry.getKey();
// reset the flag in front to avoid missing roll request before we
return from rollWriter.
- walNeedsRoll.put(wal, Boolean.FALSE);
+ controller.finishRoll();
Review comment:
Ideally we need this call also within a synchronized block. This is an
existing issue.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##########
@@ -174,18 +170,21 @@ public void run() {
}
}
try {
- this.lastRollTime = System.currentTimeMillis();
- for (Iterator<Entry<WAL, Boolean>> iter =
walNeedsRoll.entrySet().iterator(); iter
- .hasNext();) {
- Entry<WAL, Boolean> entry = iter.next();
+ for (Iterator<Entry<WAL, RollController>> iter =
walNeedsRoll.entrySet().iterator();
+ iter.hasNext();) {
+ Entry<WAL, RollController> entry = iter.next();
+ RollController controller = entry.getValue();
+ if (!controller.isRequestRoll && !controller.isPeriodRoll(now)) {
Review comment:
We can have a single method in RollController which says this WAL needs
roll? RollController#needsRoll().
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/LogRoller.java
##########
@@ -66,6 +67,10 @@ protected void scheduleFlush(String encodedRegionName,
List<byte[]> families) {
@VisibleForTesting
Map<WAL, Boolean> getWalNeedsRoll() {
Review comment:
This is exposed for test cases only. This is a private class.. So its
ok to change the return type. The new RollController gives clear idea whether
a wal instance needs roll because of periodic roll or being explicitly asked
for. So that is better.
A return type of Map<WAL, RollController>
Any way then we dont need synchronized block. Else, if we have to do as
what is being done below in patch, we would need synchronized block
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##########
@@ -148,16 +146,14 @@ private void abort(String reason, Throwable cause) {
@Override
public void run() {
while (running) {
- boolean periodic = false;
long now = System.currentTimeMillis();
checkLowReplication(now);
- periodic = (now - this.lastRollTime) > this.rollPeriod;
- if (periodic) {
+ if (walNeedsRoll.values().stream().anyMatch(rc -> rc.isPeriodRoll(now)))
{
// Time for periodic roll, fall through
LOG.debug("WAL roll period {} ms elapsed", this.rollPeriod);
Review comment:
This log is not much value added now as we dont say for which log(s)
need this periodic roll. We can clearly say WAL roll period {} elapsed for one
of the WAL.
Below we can make sure we log which wal(s) are getting rolled for what
purpose
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##########
@@ -58,14 +57,13 @@
protected static final String WAL_ROLL_PERIOD_KEY =
"hbase.regionserver.logroll.period";
- protected final ConcurrentMap<WAL, Boolean> walNeedsRoll = new
ConcurrentHashMap<>();
+ protected final ConcurrentMap<WAL, RollController> walNeedsRoll = new
ConcurrentHashMap<>();
Review comment:
We add all WAL instances into this once it is created. We can just call
it wals?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractWALRoller.java
##########
@@ -148,16 +146,14 @@ private void abort(String reason, Throwable cause) {
@Override
public void run() {
while (running) {
- boolean periodic = false;
long now = System.currentTimeMillis();
checkLowReplication(now);
- periodic = (now - this.lastRollTime) > this.rollPeriod;
- if (periodic) {
+ if (walNeedsRoll.values().stream().anyMatch(rc -> rc.isPeriodRoll(now)))
{
// Time for periodic roll, fall through
LOG.debug("WAL roll period {} ms elapsed", this.rollPeriod);
} else {
synchronized (this) {
- if (walNeedsRoll.values().stream().anyMatch(Boolean::booleanValue)) {
+ if (walNeedsRoll.values().stream().anyMatch(rc -> rc.isRequestRoll))
{
// WAL roll requested, fall through
LOG.debug("WAL roll requested");
Review comment:
Same as above comment
----------------------------------------------------------------
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]
