Uh oh!
There was an error while loading. Please reload this page.
HBASE-28467: Add time-based priority caching checks for cacheOnRead code paths. - #5905
Merged
Merged
Conversation
Comment on lines
1360
to
1352
| Optional<Boolean> cacheFileBlock = Optional.of(true); | ||
| // Additionally perform the time-based priority checks to see | ||
| // whether, or not to cache the block. | ||
| if (cacheConf.getBlockCache().isPresent()) { | ||
| cacheFileBlock = cacheConf.getBlockCache().get().shouldCacheFile(getHFileInfo(), conf); | ||
| } | ||
| final boolean shouldCacheFileBlock = cacheFileBlock.get(); | ||
Contributor
There was a problem hiding this comment.
Can we move this inside cacheConf.shouldCacheBlockOnRead?
ContributorAuthor
There was a problem hiding this comment.
This API shouldCacheBlockOnRead, is used at multiple places and takes only one argument (category). If I move the logic to this function, I will have to additionally pass these two arguments (getHFileInfo(), conf) to all usages. Hence, I will work on creating a new API within CacheConfig which internally checks for the data hotness and returns whether or not, to cache the file block.
Contributor
There was a problem hiding this comment.
I'm fine with have multiple shouldCache methods, or even overloaded versions of shouldCacheBlockOnRead, but can't we have one shouldCache like method in CacheConfig that combines both checks? That way HFileReaderImpl code doesn't need to reference multiple checks and add different boolean variables each time it needs to check it.
Apache-HBase
commented
May 15, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 15, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 15, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 16, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 16, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 16, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 16, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 16, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 16, 2024
💔 -1 overall
This message was automatically generated. |
| public boolean shouldCacheFileBlock(HFileInfo hFileInfo, Configuration conf) { | ||
| Optional<Boolean> cacheFileBlock = Optional.of(true); | ||
| // Additionally perform the time-based priority checks to see |
Contributor
There was a problem hiding this comment.
nit: Can we avoid mentioning that we are performing time-based priority checks here? At this point, we don't know that.
| if (getBlockCache().isPresent()) { | ||
| cacheFileBlock = getBlockCache().get().shouldCacheFile(hFileInfo, conf); | ||
| LOG.info("BlockCache Present, cacheFileBlock: {}", cacheFileBlock.get()); |
Contributor
There was a problem hiding this comment.
Nit: Can we change the log level to debug? This information might only be needed during debugging, and since it is logged for each block, it could generate a lot of logs.
Additionally, is the log information sufficient? It only shows true or false but doesn't indicate which file or block this information corresponds to. It will be a bunch of lines with just 'BlockCache Present, cacheFileBlock: true/false,' not revealing any detailed information.
Also, I am not sure this log line is needed because shouldCacheFile(hFileInfo, conf) already logs this information.
ContributorAuthor
There was a problem hiding this comment.
I had added to to debug something. Needs to be cleaned up. thanks for pointing this out.
| @Test | ||
| public void testCacheOnReadHotFile() throws Exception { | ||
| // hStoreFiles[0] is a hot file. the blocks should not get loaded after a readBlock call. |
Contributor
There was a problem hiding this comment.
nit: I think it should say "the blocks should get loaded after a readBlock call."
ContributorAuthor
There was a problem hiding this comment.
right! copy paste error from the above test.
Apache-HBase
commented
May 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 17, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 17, 2024
🎊 +1 overall
This message was automatically generated. |
Comment on lines
+1359
to
+1363
| final boolean cacheOnRead = cacheConf.shouldCacheBlockOnRead(category); | ||
| final boolean shouldCacheFileBlock = cacheConf.shouldCacheFileBlock(getHFileInfo(), conf); | ||
| // Don't need the unpacked block back and we're storing the block in the cache compressed | ||
| if (cacheOnly && cacheCompressed && cacheOnRead) { | ||
| if (cacheOnly && cacheCompressed && cacheOnRead && shouldCacheFileBlock) { |
Contributor
There was a problem hiding this comment.
Still in the same line of my previous comments: why do we need these two separate booleans (cacheOnRead && shouldCacheFileBlock) here? We should merge the data tiering logic inside shouldCacheBlockOnRead and deal with a single boolean variable.
ContributorAuthor
There was a problem hiding this comment.
ack. I have added an overloaded function to be used in this case and kept the original function to be used at other places. Please take a look.
Thanks,
Janardhan
jhungundforce-pushed
the
HBASE-28467
branch
2 times, most recently
from
May 21, 2024 06:44
1778877 to
1f88a38CompareApache-HBase
commented
May 21, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 21, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 21, 2024
💔 -1 overall
This message was automatically generated. |
wchevreuil
requested changes
May 21, 2024
wchevreuil
left a comment
Contributor
There was a problem hiding this comment.
Code wise, seems good to me, but the UT failures are apparently due to these changes and these tests need to be fixed.
Also, please rebase the PR with the feature branch, as it's showing conflicts after I had rebased HBASE-28463 branch with master.
…ode paths. Whenever the blocks of the file are read from the file system, they are cached into the block-cache if cache-on-read configuration is enabled. This configuration is enabled by default. However, with the time-based priority caching, we may not want to cache the blocks of the cold file. In this PR, we add checks to avoid the caching of cold data files and cache the blocks of hot data files. Change-Id: Ia07d791a841640e468c3ba33acebba81dbecbbc2
Change-Id: Ife4f3a50395877027df717191befc2962deb692d
Change-Id: Ia55e2e6ee415fa68d1c2714d127670f6df9c404d
Change-Id: I924b06e94c83502b39418097813da06d000dc336
Apache-HBase
commented
May 22, 2024
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 22, 2024
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 22, 2024
💔 -1 overall
This message was automatically generated. |
wchevreuil
approved these changes
May 22, 2024
jhungund added a commit
to janardhanrh/hbase
that referenced
this pull request
May 29, 2024
…ode paths. (apache#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Change-Id: Id2f7dff9059ac70495f162a454e4002ce51aa4be
wchevreuil pushed a commit
that referenced
this pull request
Jun 24, 2025
…ode paths. (#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
that referenced
this pull request
Jul 10, 2025
…ode paths. (#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
that referenced
this pull request
Jul 11, 2025
…ode paths. (#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
that referenced
this pull request
Aug 5, 2025
…ode paths. (#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
that referenced
this pull request
Sep 1, 2025
…ode paths. (#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Sep 1, 2025
…ode paths. (apache#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Sep 1, 2025
…ode paths. (apache#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
that referenced
this pull request
Sep 1, 2025
…ode paths. (#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
that referenced
this pull request
Sep 3, 2025
…ode paths. (#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Sep 3, 2025
…ode paths. (apache#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
wchevreuil pushed a commit
that referenced
this pull request
Sep 3, 2025
…ode paths. (#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
stoty pushed a commit
to stoty/hbase
that referenced
this pull request
Nov 22, 2025
…into cdh_main HBASE-28467: Add time-based priority caching checks for cacheOnRead code paths. (apache#5905) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Change-Id: I739d477e89966212862274612f3338f4053be023
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Whenever the blocks of the file are read from the file system, they are cached into the block-cache if cache-on-read configuration is enabled. This configuration is enabled by default. However, with the time-based priority caching, we may not want to cache the blocks of the cold file.
In this PR, we add checks to avoid the caching of cold data files and cache the blocks of hot data files.
Change-Id: Ia07d791a841640e468c3ba33acebba81dbecbbc2