HBASE-27313: Persist list of Hfiles names for which prefetch is done #4726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
requested changes
Aug 25, 2022
| @@ -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<>(); | |||
There was a problem hiding this 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?
There was a problem hiding this comment.
Thanks for the review Wellington :)
Yes, my initial approach was to use a set. But I could find a protobuf support for HashSet.
Also, I wanted to maintain a unique set of filenames so I could avoid doing a duplicate check. So I chose a map implementation.
https://developers.google.com/protocol-buffers/docs/reference/java-generated#map-fields
| @@ -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()){ | |||
There was a problem hiding this comment.
nit: No need for this extra check, as prefetchCompleted.containsKey would return false anyways if the map is empty.
| @@ -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"; | |||
There was a problem hiding this 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.
| @@ -239,6 +242,8 @@ public class BucketCache implements BlockCache, HeapSize { | |||
| /** In-memory bucket size */ | |||
| private float memoryFactor; | |||
|
|
|||
| private String prefetchPersistencePath; | |||
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
reviewed
Sep 1, 2022
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
retest |
wchevreuil
approved these changes
Sep 7, 2022
|
🎊 +1 overall
This message was automatically generated. |
|
Latest UT failures are unrelated, have tested the PR locally and have these two passing. |
taklwu
reviewed
Sep 8, 2022
| @@ -93,6 +93,8 @@ public class CacheConfig { | |||
| public static final String DROP_BEHIND_CACHE_COMPACTION_KEY = | |||
| "hbase.hfile.drop.behind.compaction"; | |||
|
|
|||
| public static final String PREFETCH_PERSISTENCE_PATH_KEY = "hbase.prefetch.file-list.path"; | |||
There was a problem hiding this comment.
[nit] it may be too late but we should not use -, "hbase.prefetch.file.list.path"
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira link: https://issues.apache.org/jira/browse/HBASE-27313
Summary:
Persist list of Hfiles names for which prefetch is completed when persist for in-memory bucket cache mapping state is enabled
We need to persist the Set, which contains a list of all the HFiles for which the prefetch has completed along with the In-memory state of the bucket cache so that when we restart and if any prefetch is scheduled again for the same HFile and we already have the in-memory state retrieved, we can skip the prefetch for it.