virajjasani commented on a change in pull request #3215:
URL: https://github.com/apache/hbase/pull/3215#discussion_r624966797



##########
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/TinyLfuBlockCache.java
##########
@@ -196,13 +202,17 @@ public void cacheBlock(BlockCacheKey key, Cacheable 
value) {
             key.getHfileName(), key.getOffset(), value.heapSize(), 
DEFAULT_MAX_BLOCK_SIZE));
       }
     } else {
+      value.retain();

Review comment:
       We can make use of `isSharedMem()` similar to how LRU does it?
   ```
     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();
     }
   ```
   And instead of directly retaining value here, we can call this method. That 
seems like the only thing we are missing?




-- 
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


Reply via email to