From 277058e18d20be76cce99b5b28dca11c9324b937 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Tue, 26 Jul 2022 16:59:57 -0400 Subject: [PATCH] HBASE-27229 BucketCache statistics should not count evictions by hfile (#4639) Signed-off-by: Andrew Purtell --- .../hbase/io/hfile/bucket/BucketCache.java | 24 +++++---- .../io/hfile/bucket/TestBucketCache.java | 52 ++++++++++++++++++- .../hfile/bucket/TestBucketCacheRefCnt.java | 12 ++--- 3 files changed, 71 insertions(+), 17 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java index b30efd53fa7..ad2ff68d8bf 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java @@ -566,13 +566,16 @@ public class BucketCache implements BlockCache, HeapSize { /** * 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()); + } } /** @@ -602,7 +605,7 @@ public class BucketCache implements BlockCache, HeapSize { */ @Override public boolean evictBlock(BlockCacheKey cacheKey) { - return doEvictBlock(cacheKey, null); + return doEvictBlock(cacheKey, null, false); } /** @@ -614,7 +617,8 @@ public class BucketCache implements BlockCache, HeapSize { * @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; } @@ -625,14 +629,14 @@ public class BucketCache implements BlockCache, HeapSize { 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; @@ -682,7 +686,7 @@ public class BucketCache implements BlockCache, HeapSize { */ boolean evictBucketEntryIfNoRpcReferenced(BlockCacheKey blockCacheKey, BucketEntry bucketEntry) { if (!bucketEntry.isRpcRef()) { - return doEvictBlock(blockCacheKey, bucketEntry); + return doEvictBlock(blockCacheKey, bucketEntry, true); } return false; } @@ -803,7 +807,7 @@ public class BucketCache implements BlockCache, HeapSize { * 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; @@ -997,7 +1001,7 @@ public class BucketCache implements BlockCache, HeapSize { 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; }); } @@ -1148,7 +1152,7 @@ public class BucketCache implements BlockCache, HeapSize { final BucketEntry bucketEntry = bucketEntries[i]; bucketEntry.withWriteLock(offsetLock, () -> { if (backingMap.remove(key, bucketEntry)) { - blockEvicted(key, bucketEntry, false); + blockEvicted(key, bucketEntry, false, false); } return null; }); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java index 66962bcd7bb..908d6ce7326 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCache.java @@ -262,7 +262,7 @@ public class TestBucketCache { }; 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])); @@ -576,6 +576,56 @@ public class TestBucketCache { 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; diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCacheRefCnt.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCacheRefCnt.java index 13f1015954d..44a398bda44 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCacheRefCnt.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketCacheRefCnt.java @@ -557,10 +557,10 @@ public class TestBucketCacheRefCnt { } @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); } /** @@ -709,8 +709,8 @@ public class TestBucketCacheRefCnt { } @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}. @@ -718,7 +718,7 @@ public class TestBucketCacheRefCnt { assertTrue(Thread.currentThread() == this.writerThreads[0]); blockEvictCounter.incrementAndGet(); - super.blockEvicted(cacheKey, bucketEntry, decrementBlockNumber); + super.blockEvicted(cacheKey, bucketEntry, decrementBlockNumber, evictedByEvictionProcess); } /**