From 4c5af0b5c0676e2c5e2ad1228be3de5a419ca736 Mon Sep 17 00:00:00 2001
From: haxiaolin <haxiaolin@apache.org>
Date: Wed, 19 May 2021 17:09:23 +0800
Subject: [PATCH] HBASE-25899 Improve efficiency of SnapshotHFileCleaner

---
 .../master/snapshot/SnapshotFileCache.java    | 40 +++++++++----------
 .../master/snapshot/SnapshotHFileCleaner.java |  2 +-
 2 files changed, 21 insertions(+), 21 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 fe46e4c97110..226627d800c9 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
@@ -20,10 +20,8 @@
 import java.io.IOException;
 import java.util.Collection;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.Timer;
 import java.util.TimerTask;
 import java.util.concurrent.locks.Lock;
@@ -36,6 +34,8 @@
 import org.apache.hadoop.hbase.snapshot.CorruptedSnapshotException;
 import org.apache.hadoop.hbase.snapshot.SnapshotDescriptionUtils;
 import org.apache.hadoop.hbase.util.CommonFSUtils;
+import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap;
+import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableSet;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.apache.yetus.audience.InterfaceStability;
 import org.slf4j.Logger;
@@ -91,12 +91,12 @@ Collection<String> filesUnderSnapshot(final FileSystem fs, final Path snapshotDi
   private final FileSystem fs, workingFs;
   private final SnapshotFileInspector fileInspector;
   private final Path snapshotDir, workingSnapshotDir;
-  private final Set<String> cache = new HashSet<>();
+  private volatile ImmutableSet<String> cache = ImmutableSet.of();
   /**
    * This is a helper map of information about the snapshot directories so we don't need to rescan
    * them if they haven't changed since the last time we looked.
    */
-  private final Map<String, SnapshotDirectoryInfo> snapshots = new HashMap<>();
+  private volatile ImmutableMap<String, SnapshotDirectoryInfo> snapshots = ImmutableMap.of();
   private final Timer refreshTimer;
 
   /**
@@ -155,7 +155,7 @@ public SnapshotFileCache(FileSystem fs, Path rootDir, FileSystem workingFs, Path
    * <p/>
    * Exposed for TESTING.
    */
-  public synchronized void triggerCacheRefreshForTesting() {
+  public void triggerCacheRefreshForTesting() {
     try {
       refreshCache();
     } catch (IOException e) {
@@ -184,7 +184,7 @@ public synchronized void triggerCacheRefreshForTesting() {
   // XXX this is inefficient to synchronize on the method, when what we really need to guard against
   // is an illegal access to the cache. Really we could do a mutex-guarded pointer swap on the
   // cache, but that seems overkill at the moment and isn't necessarily a bottleneck.
-  public synchronized Iterable<FileStatus> getUnreferencedFiles(Iterable<FileStatus> files,
+  public Iterable<FileStatus> getUnreferencedFiles(Iterable<FileStatus> files,
       final SnapshotManager snapshotManager) throws IOException {
     List<FileStatus> unReferencedFiles = Lists.newArrayList();
     List<String> snapshotsInProgress = null;
@@ -226,7 +226,7 @@ public synchronized Iterable<FileStatus> getUnreferencedFiles(Iterable<FileStatu
     return unReferencedFiles;
   }
 
-  private void refreshCache() throws IOException {
+  private synchronized void refreshCache() throws IOException {
     // just list the snapshot directory directly, do not check the modification time for the root
     // snapshot directory, as some file system implementations do not modify the parent directory's
     // modTime when there are new sub items, for example, S3.
@@ -235,22 +235,23 @@ private void refreshCache() throws IOException {
 
     // clear the cache, as in the below code, either we will also clear the snapshots, or we will
     // refill the file name cache again.
-    this.cache.clear();
     if (ArrayUtils.isEmpty(snapshotDirs)) {
       // remove all the remembered snapshots because we don't have any left
       if (LOG.isDebugEnabled() && this.snapshots.size() > 0) {
         LOG.debug("No snapshots on-disk, clear cache");
       }
-      this.snapshots.clear();
+      this.snapshots = ImmutableMap.of();
+      this.cache = ImmutableSet.of();
       return;
     }
 
+    ImmutableSet.Builder<String> cacheBuilder = ImmutableSet.builder();
+    ImmutableMap.Builder<String, SnapshotDirectoryInfo> snapshotsBuilder = ImmutableMap.builder();
     // iterate over all the cached snapshots and see if we need to update some, it is not an
     // expensive operation if we do not reload the manifest of snapshots.
-    Map<String, SnapshotDirectoryInfo> newSnapshots = new HashMap<>();
     for (FileStatus snapshotDir : snapshotDirs) {
       String name = snapshotDir.getPath().getName();
-      SnapshotDirectoryInfo files = this.snapshots.remove(name);
+      SnapshotDirectoryInfo files = snapshots.get(name);
       // if we don't know about the snapshot or its been modified, we need to update the
       // files the latter could occur where I create a snapshot, then delete it, and then make a
       // new snapshot with the same name. We will need to update the cache the information from
@@ -262,19 +263,20 @@ private void refreshCache() throws IOException {
         files = new SnapshotDirectoryInfo(snapshotDir.getModificationTime(), storedFiles);
       }
       // add all the files to cache
-      this.cache.addAll(files.getFiles());
-      newSnapshots.put(name, files);
+      cacheBuilder.addAll(files.getFiles());
+      snapshotsBuilder.put(name, files);
     }
     // set the snapshots we are tracking
-    this.snapshots.clear();
-    this.snapshots.putAll(newSnapshots);
+    this.snapshots = snapshotsBuilder.build();
+    this.cache = cacheBuilder.build();
   }
 
   List<String> getSnapshotsInProgress() throws IOException {
     List<String> snapshotInProgress = Lists.newArrayList();
     // only add those files to the cache, but not to the known snapshots
 
-    FileStatus[] snapshotsInProgress = CommonFSUtils.listStatus(this.workingFs, this.workingSnapshotDir);
+    FileStatus[] snapshotsInProgress = CommonFSUtils.listStatus(this.workingFs,
+      this.workingSnapshotDir);
 
     if (!ArrayUtils.isEmpty(snapshotsInProgress)) {
       for (FileStatus snapshot : snapshotsInProgress) {
@@ -301,11 +303,10 @@ public void run() {
         } catch (IOException e) {
           LOG.warn("Failed to refresh snapshot hfile cache!", e);
           // clear all the cached entries if we meet an error
-          cache.clear();
-          snapshots.clear();
+          cache = ImmutableSet.of();
+          snapshots = ImmutableMap.of();
         }
       }
-
     }
   }
 
@@ -315,7 +316,6 @@ public void stop(String why) {
       this.stop = true;
       this.refreshTimer.cancel();
     }
-
   }
 
   @Override
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotHFileCleaner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotHFileCleaner.java
index 7ac788894549..cc50339fd449 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotHFileCleaner.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotHFileCleaner.java
@@ -63,7 +63,7 @@ public class SnapshotHFileCleaner extends BaseHFileCleanerDelegate {
   private MasterServices master;
 
   @Override
-  public synchronized Iterable<FileStatus> getDeletableFiles(Iterable<FileStatus> files) {
+  public Iterable<FileStatus> getDeletableFiles(Iterable<FileStatus> files) {
     try {
       return cache.getUnreferencedFiles(files, master.getSnapshotManager());
     } catch (CorruptedSnapshotException cse) {