Apache9 commented on a change in pull request #3280:
URL: https://github.com/apache/hbase/pull/3280#discussion_r636113573
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
##########
@@ -91,12 +91,12 @@
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 volatile ImmutableMap<String, SnapshotDirectoryInfo> snapshots =
ImmutableMap.of();
Review comment:
We will only access this in the RefreshCacheTask thread, so do not need
to be volatile?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
##########
@@ -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,
Review comment:
I think we should store cache into a local variable and use it in the
method, as it may be switched during the method execution.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotFileCache.java
##########
@@ -226,7 +226,7 @@ public synchronized void triggerCacheRefreshForTesting() {
return unReferencedFiles;
}
- private void refreshCache() throws IOException {
+ private synchronized void refreshCache() throws IOException {
Review comment:
We do not need to make it synchronized? We will only call it below in
RefreshCacheTask and we have synchronized there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
