(hbase) branch branch-3 updated: HBASE-28211 BucketCache.blocksByHFile may leak on allocationFailure or if we reach io errors tolerated (#5530)

[wchevreuil]

This is an automated email from the ASF dual-hosted git repository.

wchevreuil pushed a commit to branch branch-3
in repository https://gitbox.apache.org/repos/asf/hbase.git


The following commit(s) were added to refs/heads/branch-3 by this push:
     new f697d0f6f2e HBASE-28211 BucketCache.blocksByHFile may leak on 
allocationFailure or if we reach io errors tolerated (#5530)
f697d0f6f2e is described below

commit f697d0f6f2e8ad500a93a3f802da42373898d996
Author: Wellington Ramos Chevreuil <wchevre...@apache.org>
AuthorDate: Wed Nov 29 10:40:33 2023 +0000

    HBASE-28211 BucketCache.blocksByHFile may leak on allocationFailure or if 
we reach io errors tolerated (#5530)
    
    Signed-off-by: Duo Zhang <zhang...@apache.org>
---
 .../org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java | 12 ++++++------
 .../hadoop/hbase/io/hfile/bucket/TestBucketWriterThread.java |  4 ++++
 2 files changed, 10 insertions(+), 6 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 ba33d5e02c4..0d510457260 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
@@ -222,7 +222,7 @@ public class BucketCache implements BlockCache, HeapSize {
    */
   transient final IdReadWriteLock<Long> offsetLock;
 
-  private final NavigableSet<BlockCacheKey> blocksByHFile = new 
ConcurrentSkipListSet<>((a, b) -> {
+  final NavigableSet<BlockCacheKey> blocksByHFile = new 
ConcurrentSkipListSet<>((a, b) -> {
     int nameComparison = a.getHfileName().compareTo(b.getHfileName());
     if (nameComparison != 0) {
       return nameComparison;
@@ -644,12 +644,14 @@ public class BucketCache implements BlockCache, HeapSize {
     blocksByHFile.remove(cacheKey);
     if (decrementBlockNumber) {
       this.blockNumber.decrement();
+      if (ioEngine.isPersistent()) {
+        removeFileFromPrefetch(cacheKey.getHfileName());
+      }
     }
     if (evictedByEvictionProcess) {
       cacheStats.evicted(bucketEntry.getCachedTime(), cacheKey.isPrimary());
     }
     if (ioEngine.isPersistent()) {
-      removeFileFromPrefetch(cacheKey.getHfileName());
       setCacheInconsistent(true);
     }
   }
@@ -1084,6 +1086,7 @@ public class BucketCache implements BlockCache, HeapSize {
    */
   protected void putIntoBackingMap(BlockCacheKey key, BucketEntry bucketEntry) 
{
     BucketEntry previousEntry = backingMap.put(key, bucketEntry);
+    blocksByHFile.add(key);
     if (previousEntry != null && previousEntry != bucketEntry) {
       previousEntry.withWriteLock(offsetLock, () -> {
         blockEvicted(key, previousEntry, false, false);
@@ -1164,10 +1167,6 @@ public class BucketCache implements BlockCache, HeapSize 
{
           index++;
           continue;
         }
-        BlockCacheKey cacheKey = re.getKey();
-        if (ramCache.containsKey(cacheKey)) {
-          blocksByHFile.add(cacheKey);
-        }
         // Reset the position for reuse.
         // It should be guaranteed that the data in the metaBuff has been 
transferred to the
         // ioEngine safely. Otherwise, this reuse is problematic. Fortunately, 
the data is already
@@ -1518,6 +1517,7 @@ public class BucketCache implements BlockCache, HeapSize {
     if (!ioEngine.isPersistent() || persistencePath == null) {
       // If persistent ioengine and a path, we will serialize out the 
backingMap.
       this.backingMap.clear();
+      this.blocksByHFile.clear();
       this.fullyCachedFiles.clear();
       this.regionCachedSizeMap.clear();
     }
diff --git 
a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketWriterThread.java
 
b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketWriterThread.java
index 4b729f33411..429fffa38f6 100644
--- 
a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketWriterThread.java
+++ 
b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestBucketWriterThread.java
@@ -122,6 +122,8 @@ public class TestBucketWriterThread {
     
Mockito.when(tooBigCacheable.getSerializedLength()).thenReturn(Integer.MAX_VALUE);
     this.bc.cacheBlock(this.plainKey, tooBigCacheable);
     doDrainOfOneEntry(this.bc, this.wt, this.q);
+    assertTrue(bc.blocksByHFile.isEmpty());
+    assertTrue(bc.getBackingMap().isEmpty());
   }
 
   /**
@@ -138,6 +140,8 @@ public class TestBucketWriterThread {
       Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
     this.q.add(spiedRqe);
     doDrainOfOneEntry(bc, wt, q);
+    assertTrue(bc.blocksByHFile.isEmpty());
+    assertTrue(bc.getBackingMap().isEmpty());
     // Cache disabled when ioes w/o ever healing.
     assertTrue(!bc.isCacheEnabled());
   }