Skip to content

Commit

Permalink
CDPD-47258 Backport HBASE-25899 Improve efficiency of SnapshotHFileCl…
Browse files Browse the repository at this point in the history
…eaner (apache#3280)

Signed-off-by: Duo Zhang <[email protected]>

(cherry picked from commit 76d56bc)

Change-Id: I74defca8babde5a941985d8e560aa1c58a5f5bd3
  • Loading branch information
sunhelly authored and Stephen Wu committed Dec 2, 2022
1 parent c5ad66f commit 3221ce5
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -91,12 +89,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 ImmutableMap<String, SnapshotDirectoryInfo> snapshots = ImmutableMap.of();
private final Timer refreshTimer;

/**
Expand Down Expand Up @@ -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<FileStatus> getUnreferencedFiles(Iterable<FileStatus> files,
final SnapshotManager snapshotManager) throws IOException {
public Iterable<FileStatus> getUnreferencedFiles(Iterable<FileStatus> files,
final SnapshotManager snapshotManager) throws IOException {
List<FileStatus> unReferencedFiles = Lists.newArrayList();
List<String> snapshotsInProgress = null;
boolean refreshed = false;
Expand All @@ -201,13 +199,17 @@ public synchronized Iterable<FileStatus> getUnreferencedFiles(Iterable<FileStatu
+ "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) {
Expand Down Expand Up @@ -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<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
Expand All @@ -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<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) {
Expand All @@ -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();
}
}

}
}

Expand All @@ -317,7 +319,6 @@ public void stop(String why) {
this.stop = true;
this.refreshTimer.cancel();
}

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 3221ce5

Please sign in to comment.