ndimiduk commented on a change in pull request #2501:
URL: https://github.com/apache/hbase/pull/2501#discussion_r503513496
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java
##########
@@ -84,8 +87,12 @@
import org.apache.yetus.audience.InterfaceAudience;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-
import
org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting;
+import
org.apache.hbase.thirdparty.com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+
Review comment:
nit: why all the extra whitespace?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java
##########
@@ -482,6 +505,8 @@ protected SyncFuture initialValue() {
this.walTooOldNs = TimeUnit.SECONDS.toNanos(conf.getInt(
SURVIVED_TOO_LONG_SEC_KEY, SURVIVED_TOO_LONG_SEC_DEFAULT));
this.useHsync = conf.getBoolean(HRegion.WAL_HSYNC_CONF_KEY,
HRegion.DEFAULT_WAL_HSYNC);
+ archiveRetries =
this.conf.getInt("hbase.regionserver.walroll.archive.retries", 0);
Review comment:
Introducing a new configuration is a hint that this is too "big/complex"
to add on a patch release.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java
##########
@@ -715,11 +740,39 @@ private void cleanOldLogs() throws IOException {
regionsBlockingThisWal.clear();
}
}
+
if (logsToArchive != null) {
- for (Pair<Path, Long> logAndSize : logsToArchive) {
- this.totalLogSize.addAndGet(-logAndSize.getSecond());
- archiveLogFile(logAndSize.getFirst());
- this.walFile2Props.remove(logAndSize.getFirst());
+ final List<Pair<Path, Long>> localLogsToArchive = logsToArchive;
+ // make it async
+ for (Pair<Path, Long> log : localLogsToArchive) {
+ logArchiveExecutor.execute(() -> {
+ archive(log);
+ });
+ this.walFile2Props.remove(log.getFirst());
+ }
+ }
+ }
+
+ protected void archive(final Pair<Path, Long> log) {
+ int retry = 1;
+ while (true) {
Review comment:
No backoff of any kind in the retry mechanism?
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/AbstractTestLogRolling.java
##########
@@ -175,10 +175,15 @@ public void testLogRollOnNothingWritten() throws
Exception {
}
}
- private void assertLogFileSize(WAL log) {
+ private void assertLogFileSize(WAL log) throws InterruptedException {
if (AbstractFSWALProvider.getNumRolledLogFiles(log) > 0) {
assertTrue(AbstractFSWALProvider.getLogFileSize(log) > 0);
} else {
+ for (int i = 0; i < 10; i++) {
Review comment:
This will be a flaky test. Please use the `waitFor` pattern already
provided on the testing utility class.
----------------------------------------------------------------
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]
