From e868be895a1e4a68a3ff66c902daa4cf10e10fc3 Mon Sep 17 00:00:00 2001 From: haxiaolin Date: Wed, 19 May 2021 17:09:23 +0800 Subject: [PATCH] HBASE-25899 Improve efficiency of SnapshotHFileCleaner --- .../master/snapshot/SnapshotFileCache.java | 48 ++++++++++--------- .../master/snapshot/SnapshotHFileCleaner.java | 2 +- 2 files changed, 27 insertions(+), 23 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..dc91515960b4 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 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; /** @@ -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 getUnreferencedFiles(Iterable files, + public Iterable getUnreferencedFiles(Iterable files, final SnapshotManager snapshotManager) throws IOException { List unReferencedFiles = Lists.newArrayList(); List snapshotsInProgress = null; @@ -200,13 +200,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) { @@ -235,22 +239,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 @@ -262,19 +267,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) { @@ -301,11 +307,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 +320,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 getDeletableFiles(Iterable files) { + public Iterable getDeletableFiles(Iterable files) { try { return cache.getUnreferencedFiles(files, master.getSnapshotManager()); } catch (CorruptedSnapshotException cse) {