virajjasani commented on a change in pull request #3215:
URL: https://github.com/apache/hbase/pull/3215#discussion_r654285359
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/TinyLfuBlockCache.java
##########
@@ -171,8 +177,10 @@ public Cacheable getBlock(BlockCacheKey cacheKey,
if ((value != null) && caching) {
if ((value instanceof HFileBlock) && ((HFileBlock)
value).isSharedMem()) {
value = HFileBlock.deepCloneOnHeap((HFileBlock) value);
+ cacheBlockUtil(cacheKey, value, true);
Review comment:
> U can do the deepclone in asReferencedHeapBlock() only based on
isSharedMem right? retain() call is anyways needed
LRUBlockCache does not perform block.retain() if block is cloned:
```
* 1. if cache the cloned heap block, its refCnt is an totally new one,
it's easy to handle; <br>
* 2. if cache the original heap block, we're sure that it won't be
tracked in ByteBuffAllocator's
* reservoir, if both RPC and LRUBlockCache release the block, then it can
be garbage collected by
* JVM, so need a retain here.
```
```
private Cacheable asReferencedHeapBlock(Cacheable buf) {
if (buf instanceof HFileBlock) {
HFileBlock blk = ((HFileBlock) buf);
if (blk.isSharedMem()) {
return HFileBlock.deepCloneOnHeap(blk);
}
}
// The block will be referenced by this LRUBlockCache, so should
increase its refCnt here.
return buf.retain();
}
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/TinyLfuBlockCache.java
##########
@@ -171,8 +177,10 @@ public Cacheable getBlock(BlockCacheKey cacheKey,
if ((value != null) && caching) {
if ((value instanceof HFileBlock) && ((HFileBlock)
value).isSharedMem()) {
value = HFileBlock.deepCloneOnHeap((HFileBlock) value);
+ cacheBlockUtil(cacheKey, value, true);
Review comment:
@anoopsjohn This is simplified now.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/TinyLfuBlockCache.java
##########
@@ -188,21 +196,58 @@ public void cacheBlock(BlockCacheKey cacheKey, Cacheable
value, boolean inMemory
@Override
public void cacheBlock(BlockCacheKey key, Cacheable value) {
+ cacheBlockUtil(key, value, false);
+ }
+
+ private void cacheBlockUtil(BlockCacheKey key, Cacheable value, boolean
deepClonedOnHeap) {
Review comment:
Done
--
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:
us...@infra.apache.org
