vinayakphegde commented on code in PR #5905:
URL: https://github.com/apache/hbase/pull/5905#discussion_r1604469915
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDataTieringManager.java:
##########
@@ -471,6 +474,62 @@ public void testFeatureKeyDisabled() throws Exception {
}
}
+ @Test
+ public void testCacheConfigShouldCacheFile() throws Exception {
+ // Evict the files from cache.
+ for (HStoreFile file : hStoreFiles) {
+ file.closeStoreFile(true);
+ }
+ // Verify that the API shouldCacheFileBlock returns the result correctly.
+ // hStoreFiles[0], hStoreFiles[1], hStoreFiles[2] are hot files.
+ // hStoreFiles[3] is a cold file.
+ try {
+
assertTrue(cacheConf.shouldCacheFileBlock(hStoreFiles.get(0).getFileInfo().getHFileInfo(),
+ hStoreFiles.get(0).getFileInfo().getConf()));
+
assertTrue(cacheConf.shouldCacheFileBlock(hStoreFiles.get(1).getFileInfo().getHFileInfo(),
+ hStoreFiles.get(1).getFileInfo().getConf()));
+
assertTrue(cacheConf.shouldCacheFileBlock(hStoreFiles.get(2).getFileInfo().getHFileInfo(),
+ hStoreFiles.get(2).getFileInfo().getConf()));
+
assertFalse(cacheConf.shouldCacheFileBlock(hStoreFiles.get(3).getFileInfo().getHFileInfo(),
+ hStoreFiles.get(3).getFileInfo().getConf()));
+ } finally {
+ for (HStoreFile file : hStoreFiles) {
+ file.initReader();
+ }
+ }
+ }
+
+ @Test
+ public void testCacheOnReadColdFile() throws Exception {
+ // hStoreFiles[3] is a cold file. the blocks should not get loaded after a
readBlock call.
+ HStoreFile hStoreFile = hStoreFiles.get(3);
+ BlockCacheKey cacheKey = new BlockCacheKey(hStoreFile.getPath(), 0, true,
BlockType.DATA);
+ testCacheOnRead(hStoreFile, cacheKey, 23025, false);
+ }
+
+ @Test
+ public void testCacheOnReadHotFile() throws Exception {
+ // hStoreFiles[0] is a hot file. the blocks should not get loaded after a
readBlock call.
Review Comment:
nit: I think it should say "the blocks should get loaded after a readBlock
call."
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java:
##########
@@ -275,6 +275,18 @@ public boolean shouldCacheBlockOnRead(BlockCategory
category) {
|| (prefetchOnOpen && (category != BlockCategory.META && category !=
BlockCategory.UNKNOWN));
}
+ public boolean shouldCacheFileBlock(HFileInfo hFileInfo, Configuration conf)
{
+ Optional<Boolean> cacheFileBlock = Optional.of(true);
+ // Additionally perform the time-based priority checks to see
+ // whether, or not to cache the block.
+ if (getBlockCache().isPresent()) {
+
+ cacheFileBlock = getBlockCache().get().shouldCacheFile(hFileInfo, conf);
+ LOG.info("BlockCache Present, cacheFileBlock: {}", cacheFileBlock.get());
Review 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.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java:
##########
@@ -275,6 +275,18 @@ public boolean shouldCacheBlockOnRead(BlockCategory
category) {
|| (prefetchOnOpen && (category != BlockCategory.META && category !=
BlockCategory.UNKNOWN));
}
+ public boolean shouldCacheFileBlock(HFileInfo hFileInfo, Configuration conf)
{
+ Optional<Boolean> cacheFileBlock = Optional.of(true);
+ // Additionally perform the time-based priority checks to see
Review Comment:
nit: Can we avoid mentioning that we are performing time-based priority
checks here? At this point, we don't know that.
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
