HBASE-27229 BucketCache statistics should not count evictions by hfile (

apache#4639)

Signed-off-by: Andrew Purtell <[email protected]>
(cherry picked from commit 25136c3)

Change-Id: I44ade79a5d6011e86c8c3f7dff9ecef72dbad3bb

hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java

@@ -576,13 +576,16 @@ public Cacheable getBlock(BlockCacheKey key, boolean caching, boolean repeat,
   /**
    * This method is invoked after the bucketEntry is removed from {@link BucketCache#backingMap}
    */
-  void blockEvicted(BlockCacheKey cacheKey, BucketEntry bucketEntry, boolean decrementBlockNumber) {
+  void blockEvicted(BlockCacheKey cacheKey, BucketEntry bucketEntry, boolean decrementBlockNumber,
+    boolean evictedByEvictionProcess) {
     bucketEntry.markAsEvicted();
     blocksByHFile.remove(cacheKey);
     if (decrementBlockNumber) {
       this.blockNumber.decrement();
     }
-    cacheStats.evicted(bucketEntry.getCachedTime(), cacheKey.isPrimary());
+    if (evictedByEvictionProcess) {
+      cacheStats.evicted(bucketEntry.getCachedTime(), cacheKey.isPrimary());
+    }
   }
 
   /**
@@ -612,7 +615,7 @@ void freeBucketEntry(BucketEntry bucketEntry) {
    */
   @Override
   public boolean evictBlock(BlockCacheKey cacheKey) {
-    return doEvictBlock(cacheKey, null);
+    return doEvictBlock(cacheKey, null, false);
   }
 
   /**
@@ -624,7 +627,8 @@ public boolean evictBlock(BlockCacheKey cacheKey) {
    * @param bucketEntry {@link BucketEntry} matched {@link BlockCacheKey} to evict.
    * @return true to indicate whether we've evicted successfully or not.
    */
-  private boolean doEvictBlock(BlockCacheKey cacheKey, BucketEntry bucketEntry) {
+  private boolean doEvictBlock(BlockCacheKey cacheKey, BucketEntry bucketEntry,
+    boolean evictedByEvictionProcess) {
     if (!cacheEnabled) {
       return false;
     }
@@ -635,14 +639,14 @@ private boolean doEvictBlock(BlockCacheKey cacheKey, BucketEntry bucketEntry) {
     final BucketEntry bucketEntryToUse = bucketEntry;
 
     if (bucketEntryToUse == null) {
-      if (existedInRamCache) {
+      if (existedInRamCache && evictedByEvictionProcess) {
         cacheStats.evicted(0, cacheKey.isPrimary());
       }
       return existedInRamCache;
     } else {
       return bucketEntryToUse.withWriteLock(offsetLock, () -> {
         if (backingMap.remove(cacheKey, bucketEntryToUse)) {
-          blockEvicted(cacheKey, bucketEntryToUse, !existedInRamCache);
+          blockEvicted(cacheKey, bucketEntryToUse, !existedInRamCache, evictedByEvictionProcess);
           return true;
         }
         return false;
@@ -692,7 +696,7 @@ public boolean evictBlockIfNoRpcReferenced(BlockCacheKey blockCacheKey) {
    */
   boolean evictBucketEntryIfNoRpcReferenced(BlockCacheKey blockCacheKey, BucketEntry bucketEntry) {
     if (!bucketEntry.isRpcRef()) {
-      return doEvictBlock(blockCacheKey, bucketEntry);
+      return doEvictBlock(blockCacheKey, bucketEntry, true);
     }
     return false;
   }
@@ -813,7 +817,7 @@ private void freeEntireBuckets(int completelyFreeBucketsNeeded) {
    * blocks evicted
    * @param why Why we are being called
    */
-  private void freeSpace(final String why) {
+  void freeSpace(final String why) {
     // Ensure only one freeSpace progress at a time
     if (!freeSpaceLock.tryLock()) {
       return;
@@ -1006,7 +1010,7 @@ protected void putIntoBackingMap(BlockCacheKey key, BucketEntry bucketEntry) {
     BucketEntry previousEntry = backingMap.put(key, bucketEntry);
     if (previousEntry != null && previousEntry != bucketEntry) {
       previousEntry.withWriteLock(offsetLock, () -> {
-        blockEvicted(key, previousEntry, false);
+        blockEvicted(key, previousEntry, false, false);
         return null;
       });
     }
@@ -1132,7 +1136,7 @@ void doDrain(final List<RAMQueueEntry> entries) throws InterruptedException {
         final BucketEntry bucketEntry = bucketEntries[i];
         bucketEntry.withWriteLock(offsetLock, () -> {
           if (backingMap.remove(key, bucketEntry)) {
-            blockEvicted(key, bucketEntry, false);
+            blockEvicted(key, bucketEntry, false, false);
           }
           return null;
         });

hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java

@@ -263,7 +263,7 @@ public void run() {
     };
     evictThread.start();
     cache.offsetLock.waitForWaiters(lockId, 1);
-    cache.blockEvicted(cacheKey, cache.backingMap.remove(cacheKey), true);
+    cache.blockEvicted(cacheKey, cache.backingMap.remove(cacheKey), true, true);
     assertEquals(0, cache.getBlockCount());
     cacheAndWaitUntilFlushedToBucket(cache, cacheKey,
       new CacheTestUtils.ByteArrayCacheable(new byte[10]), false);
@@ -482,6 +482,56 @@ public void testOffsetProducesPositiveOutput() {
     assertEquals(testValue, bucketEntry.offset());
   }
 
+  @Test
+  public void testEvictionCount() throws InterruptedException {
+    int size = 100;
+    int length = HConstants.HFILEBLOCK_HEADER_SIZE + size;
+    ByteBuffer buf1 = ByteBuffer.allocate(size), buf2 = ByteBuffer.allocate(size);
+    HFileContext meta = new HFileContextBuilder().build();
+    ByteBuffAllocator allocator = ByteBuffAllocator.HEAP;
+    HFileBlock blockWithNextBlockMetadata = new HFileBlock(BlockType.DATA, size, size, -1,
+      ByteBuff.wrap(buf1), HFileBlock.FILL_HEADER, -1, 52, -1, meta, allocator);
+    HFileBlock blockWithoutNextBlockMetadata = new HFileBlock(BlockType.DATA, size, size, -1,
+      ByteBuff.wrap(buf2), HFileBlock.FILL_HEADER, -1, -1, -1, meta, allocator);
+
+    BlockCacheKey key = new BlockCacheKey("testEvictionCount", 0);
+    ByteBuffer actualBuffer = ByteBuffer.allocate(length);
+    ByteBuffer block1Buffer = ByteBuffer.allocate(length);
+    ByteBuffer block2Buffer = ByteBuffer.allocate(length);
+    blockWithNextBlockMetadata.serialize(block1Buffer, true);
+    blockWithoutNextBlockMetadata.serialize(block2Buffer, true);
+
+    // Add blockWithNextBlockMetadata, expect blockWithNextBlockMetadata back.
+    CacheTestUtils.getBlockAndAssertEquals(cache, key, blockWithNextBlockMetadata, actualBuffer,
+      block1Buffer);
+
+    waitUntilFlushedToBucket(cache, key);
+
+    assertEquals(0, cache.getStats().getEvictionCount());
+
+    // evict call should return 1, but then eviction count be 0
+    assertEquals(1, cache.evictBlocksByHfileName("testEvictionCount"));
+    assertEquals(0, cache.getStats().getEvictionCount());
+
+    // add back
+    CacheTestUtils.getBlockAndAssertEquals(cache, key, blockWithNextBlockMetadata, actualBuffer,
+      block1Buffer);
+    waitUntilFlushedToBucket(cache, key);
+
+    // should not increment
+    assertTrue(cache.evictBlock(key));
+    assertEquals(0, cache.getStats().getEvictionCount());
+
+    // add back
+    CacheTestUtils.getBlockAndAssertEquals(cache, key, blockWithNextBlockMetadata, actualBuffer,
+      block1Buffer);
+    waitUntilFlushedToBucket(cache, key);
+
+    // should finally increment eviction count
+    cache.freeSpace("testing");
+    assertEquals(1, cache.getStats().getEvictionCount());
+  }
+
   @Test
   public void testCacheBlockNextBlockMetadataMissing() throws Exception {
     int size = 100;

hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCacheRefCnt.java

@@ -555,10 +555,10 @@ protected void cacheBlockWithWaitInternal(BlockCacheKey cacheKey, Cacheable cach
     }
 
     @Override
-    void blockEvicted(BlockCacheKey cacheKey, BucketEntry bucketEntry,
-      boolean decrementBlockNumber) {
+    void blockEvicted(BlockCacheKey cacheKey, BucketEntry bucketEntry, boolean decrementBlockNumber,
+      boolean evictedByEvictionProcess) {
       blockEvictCounter.incrementAndGet();
-      super.blockEvicted(cacheKey, bucketEntry, decrementBlockNumber);
+      super.blockEvicted(cacheKey, bucketEntry, decrementBlockNumber, evictedByEvictionProcess);
     }
 
     /**
@@ -707,16 +707,16 @@ protected boolean removeFromRamCache(BlockCacheKey cacheKey) {
     }
 
     @Override
-    void blockEvicted(BlockCacheKey cacheKey, BucketEntry bucketEntry,
-      boolean decrementBlockNumber) {
+    void blockEvicted(BlockCacheKey cacheKey, BucketEntry bucketEntry, boolean decrementBlockNumber,
+      boolean evictedByEvictionProcess) {
       /**
        * This is only invoked by {@link BucketCache.WriterThread}. {@link MyMyBucketCache2} create
        * only one {@link BucketCache.WriterThread}.
        */
       assertTrue(Thread.currentThread() == this.writerThreads[0]);
 
       blockEvictCounter.incrementAndGet();
-      super.blockEvicted(cacheKey, bucketEntry, decrementBlockNumber);
+      super.blockEvicted(cacheKey, bucketEntry, decrementBlockNumber, evictedByEvictionProcess);
     }
 
     /**