From f81bb8b6ecf1012c7d489cadc89ea96ebd66e69c Mon Sep 17 00:00:00 2001
From: Xiaolin Ha <haxiaolin@apache.org>
Date: Sat, 21 May 2022 06:32:58 +0800
Subject: [PATCH] HBASE-27043 Let lock wait timeout to improve performance of
 SnapshotHFileCleaner (#4437)

Signed-off-by: Andrew Purtell <apurtell@apache.org>
---
 .../master/snapshot/SnapshotFileCache.java    | 67 +++++++++++--------
 1 file changed, 39 insertions(+), 28 deletions(-)

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
index 10a7703c9ff4..5d9701d9bcc8 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
@@ -22,6 +22,7 @@
 import java.util.List;
 import java.util.Timer;
 import java.util.TimerTask;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.Lock;
 import org.apache.commons.lang3.ArrayUtils;
 import org.apache.hadoop.conf.Configuration;
@@ -97,6 +98,8 @@ Collection<String> filesUnderSnapshot(final FileSystem fs, final Path snapshotDi
   private ImmutableMap<String, SnapshotDirectoryInfo> snapshots = ImmutableMap.of();
   private final Timer refreshTimer;
 
+  private static final int LOCK_TIMEOUT_MS = 30000;
+
   /**
    * Create a snapshot file cache for all snapshots under the specified [root]/.snapshot on the
    * filesystem.
@@ -192,39 +195,47 @@ public Iterable<FileStatus> getUnreferencedFiles(Iterable<FileStatus> files,
     if (snapshotManager != null) {
       lock = snapshotManager.getTakingSnapshotLock().writeLock();
     }
-    if (lock == null || lock.tryLock()) {
-      try {
-        if (snapshotManager != null && snapshotManager.isTakingAnySnapshot()) {
-          LOG.warn("Not checking unreferenced files since snapshot is running, it will "
-            + "skip to clean the HFiles this time");
-          return unReferencedFiles;
-        }
-        ImmutableSet<String> currentCache = cache;
-        for (FileStatus file : files) {
-          String fileName = file.getPath().getName();
-          if (!refreshed && !currentCache.contains(fileName)) {
-            synchronized (this) {
-              refreshCache();
-              currentCache = cache;
-              refreshed = true;
-            }
-          }
-          if (currentCache.contains(fileName)) {
-            continue;
+    try {
+      if (lock == null || lock.tryLock(LOCK_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
+        try {
+          if (snapshotManager != null && snapshotManager.isTakingAnySnapshot()) {
+            LOG.warn("Not checking unreferenced files since snapshot is running, it will "
+              + "skip to clean the HFiles this time");
+            return unReferencedFiles;
           }
-          if (snapshotsInProgress == null) {
-            snapshotsInProgress = getSnapshotsInProgress();
+          ImmutableSet<String> currentCache = cache;
+          for (FileStatus file : files) {
+            String fileName = file.getPath().getName();
+            if (!refreshed && !currentCache.contains(fileName)) {
+              synchronized (this) {
+                refreshCache();
+                currentCache = cache;
+                refreshed = true;
+              }
+            }
+            if (currentCache.contains(fileName)) {
+              continue;
+            }
+            if (snapshotsInProgress == null) {
+              snapshotsInProgress = getSnapshotsInProgress();
+            }
+            if (snapshotsInProgress.contains(fileName)) {
+              continue;
+            }
+            unReferencedFiles.add(file);
           }
-          if (snapshotsInProgress.contains(fileName)) {
-            continue;
+        } finally {
+          if (lock != null) {
+            lock.unlock();
           }
-          unReferencedFiles.add(file);
-        }
-      } finally {
-        if (lock != null) {
-          lock.unlock();
         }
+      } else {
+        LOG.warn("Failed to acquire write lock on taking snapshot after waiting {}ms",
+          LOCK_TIMEOUT_MS);
       }
+    } catch (InterruptedException e) {
+      LOG.warn("Interrupted while acquiring write lock on taking snapshot");
+      Thread.currentThread().interrupt(); // restore the interrupt flag
     }
     return unReferencedFiles;
   }