This is an automated email from the ASF dual-hosted git repository. psomogyi pushed a commit to branch branch-2.4 in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-2.4 by this push: new 1843cefa989 HBASE-27627 Improve efficiency of SnapshotHFileCleaner (#5017) 1843cefa989 is described below commit 1843cefa989ab2e157626471e40b115696f88402 Author: Peter Somogyi <psomo...@apache.org> AuthorDate: Thu Feb 9 19:07:56 2023 +0100 HBASE-27627 Improve efficiency of SnapshotHFileCleaner (#5017) Backport of HBASE-25899 Signed-off-by: Tak Lon (Stephen) Wu <tak...@apache.org> --- .../hbase/master/snapshot/SnapshotFileCache.java | 47 +++++++++++----------- .../master/snapshot/SnapshotHFileCleaner.java | 2 +- 2 files changed, 25 insertions(+), 24 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 2f669ae8bc5..f9f948ff8a2 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 @@ package org.apache.hadoop.hbase.master.snapshot; 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; @@ -41,6 +37,8 @@ import org.apache.yetus.audience.InterfaceStability; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap; +import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableSet; import org.apache.hbase.thirdparty.com.google.common.collect.Lists; /** @@ -91,12 +89,12 @@ public class SnapshotFileCache implements Stoppable { 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 ImmutableMap<String, SnapshotDirectoryInfo> snapshots = ImmutableMap.of(); private final Timer refreshTimer; /** @@ -185,7 +183,7 @@ public class SnapshotFileCache implements Stoppable { // 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; @@ -201,13 +199,17 @@ public class SnapshotFileCache implements Stoppable { + "skip to clean the HFiles this time"); return unReferencedFiles; } + ImmutableSet<String> 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 @@ public class SnapshotFileCache implements Stoppable { // 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 @@ -263,12 +266,12 @@ public class SnapshotFileCache implements Stoppable { 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 { @@ -303,11 +306,10 @@ public class SnapshotFileCache implements Stoppable { } 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 class SnapshotFileCache implements Stoppable { 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 0cd86f933dc..a300cbbce68 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) {