From 3221ce51ad40a66d840e39b205d53bceb05d338d Mon Sep 17 00:00:00 2001 From: Xiaolin Ha Date: Mon, 24 May 2021 22:15:47 +0800 Subject: [PATCH] CDPD-47258 Backport HBASE-25899 Improve efficiency of SnapshotHFileCleaner (#3280) Signed-off-by: Duo Zhang (cherry picked from commit 76d56bc42e270f77af896c849908ad32092f4d84) Change-Id: I74defca8babde5a941985d8e560aa1c58a5f5bd3 --- .../master/snapshot/SnapshotFileCache.java | 53 ++++++++++--------- .../master/snapshot/SnapshotHFileCleaner.java | 2 +- 2 files changed, 28 insertions(+), 27 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 2f669ae8bc5f..030cdabdc4b5 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 @@ -19,11 +19,7 @@ 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 +32,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 +89,12 @@ Collection filesUnderSnapshot(final FileSystem fs, final Path snapshotDi private final FileSystem fs, workingFs; private final SnapshotFileInspector fileInspector; private final Path snapshotDir, workingSnapshotDir; - private final Set cache = new HashSet<>(); + private volatile ImmutableSet 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 snapshots = new HashMap<>(); + private ImmutableMap snapshots = ImmutableMap.of(); private final Timer refreshTimer; /** @@ -185,8 +183,8 @@ 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 getUnreferencedFiles(Iterable files, - final SnapshotManager snapshotManager) throws IOException { + public Iterable getUnreferencedFiles(Iterable files, + final SnapshotManager snapshotManager) throws IOException { List unReferencedFiles = Lists.newArrayList(); List snapshotsInProgress = null; boolean refreshed = false; @@ -201,13 +199,17 @@ public synchronized Iterable getUnreferencedFiles(Iterable currentCache = cache; for (FileStatus file : files) { String fileName = file.getPath().getName(); - if (!refreshed && !cache.contains(fileName)) { - refreshCache(); - refreshed = true; + if (!refreshed && !currentCache.contains(fileName)) { + synchronized (this) { + refreshCache(); + currentCache = cache; + refreshed = true; + } } - if (cache.contains(fileName)) { + if (currentCache.contains(fileName)) { continue; } if (snapshotsInProgress == null) { @@ -236,22 +238,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 cacheBuilder = ImmutableSet.builder(); + ImmutableMap.Builder 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 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 @@ -263,20 +266,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 getSnapshotsInProgress() throws IOException { List 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) { @@ -303,11 +306,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(); } } - } } @@ -317,7 +319,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 0cd86f933dc6..a300cbbce68b 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 getDeletableFiles(Iterable files) { + public Iterable getDeletableFiles(Iterable files) { try { return cache.getUnreferencedFiles(files, master.getSnapshotManager()); } catch (CorruptedSnapshotException cse) {