Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-25899 Improve efficiency of SnapshotHFileCleaner #3280

Merged
merged 1 commit into from
May 24, 2021
+27 −23
Merged

HBASE-25899 Improve efficiency of SnapshotHFileCleaner #3280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 26 additions & 22 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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 ImmutableMap<String, SnapshotDirectoryInfo> snapshots = ImmutableMap.of();
private final Timer refreshTimer;

/**
Expand Down Expand Up @@ -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,
sunhelly marked this conversation as resolved.
Show resolved Hide resolved
final SnapshotManager snapshotManager) throws IOException {
List<FileStatus> unReferencedFiles = Lists.newArrayList();
List<String> snapshotsInProgress = null;
Expand All @@ -200,13 +200,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;
}
}
Comment on lines +207 to 212
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this line is still blocking the delete tasks... basically I LOG.info/print the lock information before the line of if (lock == null || lock.tryLock()) { , I found it won't add any unReferencedFiles file because once a synchronized refresh cache in line 207 is being called e.g. java.util.concurrent.locks.ReentrantReadWriteLock$WriteLock@568e37c0[Locked by thread hfile_cleaner-dir-scan-pool-2] , then whatever files passing in will be skipped.

maybe I'm wondered, why do we need a write lock for refreshing snapshot file cache ?

if (cache.contains(fileName)) {
if (currentCache.contains(fileName)) {
continue;
}
if (snapshotsInProgress == null) {
Expand Down Expand Up @@ -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<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 @@ -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<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 @@ -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();
}
}

}
}

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

}

@Override
Expand Down
2 changes: 1 addition & 1 deletion hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotHFileCleaner.java
View file
Open in desktop
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