wchevreuil commented on code in PR #4726:
URL: https://github.com/apache/hbase/pull/4726#discussion_r954813261
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -239,6 +242,8 @@ public class BucketCache implements BlockCache, HeapSize {
/** In-memory bucket size */
private float memoryFactor;
+ private String prefetchPersistencePath;
Review Comment:
See comment above.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/PrefetchExecutor.java:
##########
@@ -44,12 +50,15 @@ public final class PrefetchExecutor {
/** Futures for tracking block prefetch activity */
private static final Map<Path, Future<?>> prefetchFutures = new
ConcurrentSkipListMap<>();
+ /** Set of files for which prefetch is completed */
+ public static HashMap<String, Boolean> prefetchCompleted = new HashMap<>();
Review Comment:
nit: do we really need a map here? It seems we never check the boolean
value, but only if the map has an entry for the path of not. Could we use
hashset or a list impl?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java:
##########
@@ -93,6 +93,8 @@ public class CacheConfig {
public static final String DROP_BEHIND_CACHE_COMPACTION_KEY =
"hbase.hfile.drop.behind.compaction";
+ public static String PREFETCH_PERSISTENCE_PATH_KEY =
"hbase.prefetch.persist.filepath";
Review Comment:
nit: So this is basically a path for a list of prefetched files. Can we call
it `hbase.prefetch.file-list.path`? That would be more intuitive, especially
when we start to use it in BucketCache class, which already has a
`persistencePath` variable. We could then relabel
`BuckeCache.prefetchPersistencePath` to something like
`BuckeCache.prefetchedFileListPath`.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/PrefetchExecutor.java:
##########
@@ -79,6 +88,12 @@ public Thread newThread(Runnable r) {
+ HConstants.HREGION_COMPACTIONDIR_NAME.replace(".", "\\.") +
Path.SEPARATOR_CHAR + ")");
public static void request(Path path, Runnable runnable) {
+ if(!prefetchCompleted.isEmpty()){
Review Comment:
nit: No need for this extra check, as `prefetchCompleted.containsKey` would
return false anyways if the map is empty.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
