anoopsjohn commented on a change in pull request #3215:
URL: https://github.com/apache/hbase/pull/3215#discussion_r624684605
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/TinyLfuBlockCache.java
##########
@@ -177,6 +177,7 @@ public Cacheable getBlock(BlockCacheKey cacheKey,
}
} else if (updateCacheMetrics) {
stats.hit(caching, cacheKey.isPrimary(), cacheKey.getBlockType());
+ value.retain();
Review comment:
We should follow the same as in LRU cache no?
LruCachedBlock cb = map.computeIfPresent(cacheKey, (key, val) -> {
// It will be referenced by RPC path, so increase here. NOTICE: Must
do the retain inside
// this block. because if retain outside the map#computeIfPresent, the
evictBlock may remove
// the block and release, then we're retaining a block with refCnt=0
which is disallowed.
// see HBASE-22422.
val.getBuffer().retain();
return val;
});
And other places
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java
##########
@@ -205,10 +206,11 @@ public void testReaderWithLRUBlockCache() throws
Exception {
lru.shutdown();
}
- private BlockCache initCombinedBlockCache() {
+ private BlockCache initCombinedBlockCache(final String l2Cache) {
Review comment:
What we pass is the l1cache's policy name. So this arg name not
appropriate
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java
##########
@@ -890,4 +892,89 @@ public void testDBEShipped() throws IOException {
writer.close();
}
}
+
+ /**
+ * Test case for CombinedBlockCache with TinyLfu as L1 cache
+ */
+ @Test
+ public void testReaderWithTinyLfuCombinedBlockCache() throws Exception {
Review comment:
These 2 are same tests cases but only diff is L1 policy name? If so can
we avoid duplication and have single private method and call with diff args?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
