From 0870efe72246fd60e50ea1102224e7c4ca91966f Mon Sep 17 00:00:00 2001 From: Wellington Ramos Chevreuil Date: Thu, 11 Jul 2024 16:39:58 +0100 Subject: [PATCH 1/2] HBASE-28724 BucketCache.notifyFileCachingCompleted may throw IllegalMonitorStateException Change-Id: Ib7311f597fcde8cf72456e0defe50ae98a2da692 --- .../hbase/io/hfile/bucket/BucketCache.java | 1 + .../hadoop/hbase/io/hfile/CacheTestUtils.java | 25 ++++++--- .../io/hfile/bucket/TestBucketCache.java | 52 +++++++++++++++++++ 3 files changed, 70 insertions(+), 8 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 8ee0b6b98ada..5816b8ff1602 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 @@ -2108,6 +2108,7 @@ public void notifyFileCachingCompleted(Path fileName, int totalBlockCount, int d for (ReentrantReadWriteLock lock : locks) { lock.readLock().unlock(); } + locks.clear(); LOG.debug("There are still blocks pending caching for file {}. Will sleep 100ms " + "and try the verification again.", fileName); Thread.sleep(100); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java index 262408e91a82..60cc6ea0f671 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java @@ -32,6 +32,7 @@ import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.atomic.AtomicInteger; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.MultithreadedTestUtil; import org.apache.hadoop.hbase.MultithreadedTestUtil.TestThread; @@ -275,6 +276,10 @@ public BlockType getBlockType() { } public static HFileBlockPair[] generateHFileBlocks(int blockSize, int numBlocks) { + return generateBlocksForPath(blockSize, numBlocks, null); + } + + public static HFileBlockPair[] generateBlocksForPath(int blockSize, int numBlocks, Path path) { HFileBlockPair[] returnedBlocks = new HFileBlockPair[numBlocks]; Random rand = ThreadLocalRandom.current(); HashSet usedStrings = new HashSet<>(); @@ -299,16 +304,20 @@ public static HFileBlockPair[] generateHFileBlocks(int blockSize, int numBlocks) prevBlockOffset, ByteBuff.wrap(cachedBuffer), HFileBlock.DONT_FILL_HEADER, blockSize, onDiskSizeWithoutHeader + HConstants.HFILEBLOCK_HEADER_SIZE, -1, meta, ByteBuffAllocator.HEAP); - - String strKey; - /* No conflicting keys */ - strKey = Long.toString(rand.nextLong()); - while (!usedStrings.add(strKey)) { - strKey = Long.toString(rand.nextLong()); + String key = null; + long offset = 0; + if (path!=null) { + key = path.getName(); + offset = i * blockSize; + } else { + /* No conflicting keys */ + key = Long.toString(rand.nextLong()); + while (!usedStrings.add(key)) { + key = Long.toString(rand.nextLong()); + } } - returnedBlocks[i] = new HFileBlockPair(); - returnedBlocks[i].blockName = new BlockCacheKey(strKey, 0); + returnedBlocks[i].blockName = new BlockCacheKey(key,offset); returnedBlocks[i].block = generated; } return returnedBlocks; 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 6a9b5bf382a6..73db3d59a933 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 @@ -890,4 +890,56 @@ public void testBlockAdditionWaitWhenCache() throws Exception { HBASE_TESTING_UTILITY.cleanupTestDir(); } } + + @Test + public void testNotifyFileCachingCompletedSuccess() throws Exception { + BucketCache bucketCache = null; + try { + Path filePath = new Path(HBASE_TESTING_UTILITY.getDataTestDir(), + "testNotifyFileCachingCompletedSuccess"); + bucketCache = testNotifyFileCachingCompleted(filePath, 10); + assertTrue(bucketCache.fullyCachedFiles.containsKey(filePath.getName())); + } finally { + if (bucketCache != null) { + bucketCache.shutdown(); + } + HBASE_TESTING_UTILITY.cleanupTestDir(); + } + } + + @Test + public void testNotifyFileCachingCompletedNotAllCached() throws Exception { + BucketCache bucketCache = null; + try { + Path filePath = new Path(HBASE_TESTING_UTILITY.getDataTestDir(), + "testNotifyFileCachingCompletedNotAllCached"); + //Deliberately passing more blocks than we have created to test that + //notifyFileCachingCompleted will not consider the file fully cached + bucketCache = testNotifyFileCachingCompleted(filePath, 12); + assertFalse(bucketCache.fullyCachedFiles.containsKey(filePath.getName())); + } finally { + if (bucketCache != null) { + bucketCache.shutdown(); + } + HBASE_TESTING_UTILITY.cleanupTestDir(); + } + } + private BucketCache testNotifyFileCachingCompleted(Path filePath, int totalBlocks) throws Exception { + final Path dataTestDir = createAndGetTestDir(); + String ioEngineName = "file:" + dataTestDir + "/bucketNoRecycler.cache"; + BucketCache bucketCache = new BucketCache(ioEngineName, capacitySize, constructedBlockSize, + constructedBlockSizes, 1, 1, null); + long usedByteSize = bucketCache.getAllocator().getUsedSize(); + assertEquals(0, usedByteSize); + HFileBlockPair[] hfileBlockPairs = + CacheTestUtils.generateBlocksForPath(constructedBlockSize, 10, filePath); + // Add blocks + for (HFileBlockPair hfileBlockPair : hfileBlockPairs) { + bucketCache.cacheBlock(hfileBlockPair.getBlockName(), hfileBlockPair.getBlock(), false, + true); + } + bucketCache.notifyFileCachingCompleted(filePath, totalBlocks, totalBlocks, + totalBlocks * constructedBlockSize); + return bucketCache; + } } From 237f02b8554b6b7193d155ba97a3fc23c639cf92 Mon Sep 17 00:00:00 2001 From: Wellington Ramos Chevreuil Date: Fri, 12 Jul 2024 12:35:26 +0100 Subject: [PATCH 2/2] spotless Change-Id: I307b7347ba260e610b35d339c728e2cee74f3880 --- .../hadoop/hbase/io/hfile/CacheTestUtils.java | 4 ++-- .../hbase/io/hfile/bucket/TestBucketCache.java | 15 ++++++++------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java index 60cc6ea0f671..848f33bb9c3a 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java @@ -306,7 +306,7 @@ public static HFileBlockPair[] generateBlocksForPath(int blockSize, int numBlock ByteBuffAllocator.HEAP); String key = null; long offset = 0; - if (path!=null) { + if (path != null) { key = path.getName(); offset = i * blockSize; } else { @@ -317,7 +317,7 @@ public static HFileBlockPair[] generateBlocksForPath(int blockSize, int numBlock } } returnedBlocks[i] = new HFileBlockPair(); - returnedBlocks[i].blockName = new BlockCacheKey(key,offset); + returnedBlocks[i].blockName = new BlockCacheKey(key, offset); returnedBlocks[i].block = generated; } return returnedBlocks; 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 73db3d59a933..78a781994e83 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 @@ -895,8 +895,8 @@ public void testBlockAdditionWaitWhenCache() throws Exception { public void testNotifyFileCachingCompletedSuccess() throws Exception { BucketCache bucketCache = null; try { - Path filePath = new Path(HBASE_TESTING_UTILITY.getDataTestDir(), - "testNotifyFileCachingCompletedSuccess"); + Path filePath = + new Path(HBASE_TESTING_UTILITY.getDataTestDir(), "testNotifyFileCachingCompletedSuccess"); bucketCache = testNotifyFileCachingCompleted(filePath, 10); assertTrue(bucketCache.fullyCachedFiles.containsKey(filePath.getName())); } finally { @@ -913,8 +913,8 @@ public void testNotifyFileCachingCompletedNotAllCached() throws Exception { try { Path filePath = new Path(HBASE_TESTING_UTILITY.getDataTestDir(), "testNotifyFileCachingCompletedNotAllCached"); - //Deliberately passing more blocks than we have created to test that - //notifyFileCachingCompleted will not consider the file fully cached + // Deliberately passing more blocks than we have created to test that + // notifyFileCachingCompleted will not consider the file fully cached bucketCache = testNotifyFileCachingCompleted(filePath, 12); assertFalse(bucketCache.fullyCachedFiles.containsKey(filePath.getName())); } finally { @@ -924,7 +924,9 @@ public void testNotifyFileCachingCompletedNotAllCached() throws Exception { HBASE_TESTING_UTILITY.cleanupTestDir(); } } - private BucketCache testNotifyFileCachingCompleted(Path filePath, int totalBlocks) throws Exception { + + private BucketCache testNotifyFileCachingCompleted(Path filePath, int totalBlocks) + throws Exception { final Path dataTestDir = createAndGetTestDir(); String ioEngineName = "file:" + dataTestDir + "/bucketNoRecycler.cache"; BucketCache bucketCache = new BucketCache(ioEngineName, capacitySize, constructedBlockSize, @@ -935,8 +937,7 @@ private BucketCache testNotifyFileCachingCompleted(Path filePath, int totalBlock CacheTestUtils.generateBlocksForPath(constructedBlockSize, 10, filePath); // Add blocks for (HFileBlockPair hfileBlockPair : hfileBlockPairs) { - bucketCache.cacheBlock(hfileBlockPair.getBlockName(), hfileBlockPair.getBlock(), false, - true); + bucketCache.cacheBlock(hfileBlockPair.getBlockName(), hfileBlockPair.getBlock(), false, true); } bucketCache.notifyFileCachingCompleted(filePath, totalBlocks, totalBlocks, totalBlocks * constructedBlockSize);