[GitHub] [hbase] ndimiduk commented on a change in pull request #2501: HBASE-25065 WAL archival to be done by a separate thread

[GitBox <gi...@apache.org>]

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