HBASE-27794: Tooling for parsing/reading the prefetch files list file #5468
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. |
wchevreuil
reviewed
Oct 18, 2023
There was a problem hiding this comment.
Thanks for adding this @Kota-SH ! I had a few nits.
Also, can we add a tool to parse the persistent cache map file and read the list of cached files from there? I think it would be good for troubleshooting if we could analyse the persistent cache file contents.
Comment on lines
3948
to
3952
| server.getBlockCache().ifPresent(blockCache -> { | ||
| if (blockCache instanceof CombinedBlockCache) { | ||
| BlockCache l2 = ((CombinedBlockCache) blockCache).getSecondLevelCache(); | ||
| if (l2 instanceof BucketCache) { | ||
| if (((BucketCache) l2).isCachePersistenceEnabled()) { |
There was a problem hiding this comment.
Use the static method BucketCache.getBucketCacheFromCacheConfig for finding the bucket cache instance.
There was a problem hiding this comment.
Thanks for the feedback @wchevreuil, However, my idea is that we have RegionServer.getBlockCache(), so I added a getter (getBucketCacheIfPresent()) for the bucket cache as a property of the RS.
WDYT?
There was a problem hiding this comment.
It's fine to add the additional getter, but we should not duplicate code. This whole implementation is identical to BucketCache.getBucketCacheFromCacheConfig, so you should just call it inside your method.
| /** | ||
| * Get the list of prefetched files | ||
| */ | ||
| List<String> getPrefetchedFilesList(ServerName serverName) throws IOException; |
There was a problem hiding this comment.
Nit: let's call it "getCachedFilesList".
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| Admin admin = TEST_UTIL.getAdmin(); | ||
| List<String> prefetchedFilesList = | ||
| admin.getPrefetchedFilesList(regionServingRS.getServerName()); | ||
| assertEquals(1, ((List<?>) prefetchedFilesList).size()); |
There was a problem hiding this comment.
Nit: (List<?>) Redundant cast can be removed as prefetchedFilesList type is List
Kota-SH
force-pushed
the
hbase-27794-n
branch
from
e1dde58 to
ccc3689
Compare
|
💔 -1 overall
This message was automatically generated. |
Kota-SH
force-pushed
the
hbase-27794-n
branch
2 times, most recently
from
70e8c80 to
6e07f26
Compare
|
🎊 +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. |
Kota-SH
force-pushed
the
hbase-27794-n
branch
from
6e07f26 to
153d50c
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Kota-SH
force-pushed
the
hbase-27794-n
branch
from
153d50c to
aa2607f
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Oct 26, 2023
Comment on lines
3948
to
3950
| CacheConfig cacheConfig = server.getRegions().get(0).getStores().get(0).getCacheConfig(); | ||
| BucketCache.getBucketCacheFromCacheConfig(cacheConfig).ifPresent(bucketCache -> { | ||
| if (bucketCache.isCachePersistenceEnabled()) { |
There was a problem hiding this comment.
Maybe we should encapsulate this in the BlockCache.
wchevreuil
reviewed
Oct 26, 2023
Comment on lines
1987
to
2000
| public List<String> getCachedFilesList() throws IOException { | ||
| List<String> tmp = new ArrayList<>(); | ||
| File persistedFile = new File(persistencePath); | ||
| try (FileInputStream fis = new FileInputStream(persistedFile)) { | ||
| if (fis.skip(ProtobufMagic.lengthOfPBMagic()) != ProtobufMagic.lengthOfPBMagic()) { | ||
| throw new IOException( | ||
| "Incorrect number of bytes read while checking for protobuf magic number"); | ||
| } | ||
| BucketCacheProtos.BucketCacheEntry proto = | ||
| BucketCacheProtos.BucketCacheEntry.parseDelimitedFrom(fis); | ||
| tmp.addAll(proto.getPrefetchedFilesMap().keySet()); | ||
| } | ||
| return tmp; | ||
| } |
There was a problem hiding this comment.
So this would make sense for a standalone tool, but since we are implementing this whole thing as part of the RS RPC API, why don't we simply return the in-memory contents?
Kota-SH
force-pushed
the
hbase-27794-n
branch
from
aa2607f to
4703d5d
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Kota-SH
force-pushed
the
hbase-27794-n
branch
from
4703d5d to
f6d0786
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Oct 31, 2023
| @@ -1965,8 +1965,8 @@ public AtomicBoolean getBackingMapValidated() { | |||
| return backingMapValidated; | |||
| } | |||
|
|
|||
| public Map<String, Boolean> getFullyCachedFiles() { | |||
| return fullyCachedFiles; | |||
| public Optional<Map<String, Boolean>> getFullyCachedFiles() { | |||
There was a problem hiding this comment.
nit: mark with Override annotation.
wchevreuil
approved these changes
Oct 31, 2023
Kota-SH
force-pushed
the
hbase-27794-n
branch
from
f6d0786 to
abf4e0c
Compare
|
🎊 +1 overall
This message was automatically generated. |
Kota-SH
force-pushed
the
hbase-27794-n
branch
from
abf4e0c to
e7ddc29
Compare
|
🎊 +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
pushed a commit
that referenced
this pull request
Nov 1, 2023
…#5468) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil
pushed a commit
that referenced
this pull request
Nov 1, 2023
…#5468) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
Kota-SH
added a commit
to Kota-SH/hbase
that referenced
this pull request
Nov 1, 2023
…apache#5468) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> (cherry picked from commit fa4c896)
Kota-SH
added a commit
to Kota-SH/hbase
that referenced
this pull request
Nov 1, 2023
…apache#5468) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> (cherry picked from commit fa4c896)
Kota-SH
added a commit
to Kota-SH/hbase
that referenced
this pull request
Nov 2, 2023
…apache#5468) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> (cherry picked from commit fa4c896)
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Nov 2, 2023
…apache#5468) Signed-off-by: Wellingt
wchevreuil
added a commit
to wchevreuil/hbase
that referenced
this pull request
Nov 2, 2023
…ist file (apache#5468)" This reverts commit c37fba6.
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Nov 2, 2023
…apache#5468) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…es list file (apache#5468) (apache#5491) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> (cherry picked from commit fa4c896) Change-Id: I2e96f4b8bccef4ca9ae518dfca2283e63ec07102
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: https://issues.apache.org/jira/browse/HBASE-27794