This is an automated email from the ASF dual-hosted git repository. taklwu pushed a commit to branch HBASE-30018 in repository https://gitbox.apache.org/repos/asf/hbase.git
commit 096cd40a104e959f5b65eab1b6d89bbf24f2a98d Author: Vladimir Rodionov <[email protected]> AuthorDate: Thu May 28 15:01:58 2026 -0700 HBASE-30022 Refactor CacheConfig and HFileReaderImpl to use CacheAccessService (#8279) Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Tak Lon (Stephen) Wu <[email protected]> Reviewed by: Kota-SH <[email protected]> --- .../apache/hadoop/hbase/io/hfile/CacheConfig.java | 25 +++ .../hadoop/hbase/io/hfile/HFilePreadReader.java | 16 +- .../hadoop/hbase/io/hfile/HFileReaderImpl.java | 200 ++++++++++----------- .../cache/BlockCacheBackedCacheAccessService.java | 8 + .../hbase/io/hfile/cache/CacheAccessService.java | 24 +++ .../hadoop/hbase/io/hfile/cache/CacheEngine.java | 13 ++ .../hadoop/hbase/io/hfile/cache/CacheTopology.java | 1 + .../io/hfile/cache/NoOpCacheAccessService.java | 7 + .../hadoop/hbase/io/hfile/TestCacheConfig.java | 32 +++- .../TestBlockCacheBackedCacheAccessService.java | 25 +++ .../io/hfile/cache/TestNoOpCacheAccessService.java | 117 ++++++++++++ 11 files changed, 360 insertions(+), 108 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java index 475a22703e1..3505f822814 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java @@ -24,6 +24,8 @@ import org.apache.hadoop.hbase.conf.ConfigurationManager; import org.apache.hadoop.hbase.conf.PropagatingConfigurationObserver; import org.apache.hadoop.hbase.io.ByteBuffAllocator; import org.apache.hadoop.hbase.io.hfile.BlockType.BlockCategory; +import org.apache.hadoop.hbase.io.hfile.cache.CacheAccessService; +import org.apache.hadoop.hbase.io.hfile.cache.CacheAccessServices; import org.apache.yetus.audience.InterfaceAudience; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -176,6 +178,8 @@ public class CacheConfig implements PropagatingConfigurationObserver { // Local reference to the block cache private final BlockCache blockCache; + private final CacheAccessService cacheAccessService; + private final ByteBuffAllocator byteBuffAllocator; private double heapUsageThreshold; @@ -231,6 +235,9 @@ public class CacheConfig implements PropagatingConfigurationObserver { } this.blockCache = blockCache; this.byteBuffAllocator = byteBuffAllocator; + this.cacheAccessService = blockCache != null + ? CacheAccessServices.fromBlockCache(blockCache) + : CacheAccessServices.disabled(); } /** @@ -252,6 +259,9 @@ public class CacheConfig implements PropagatingConfigurationObserver { this.blockCache = cacheConf.blockCache; this.byteBuffAllocator = cacheConf.byteBuffAllocator; this.heapUsageThreshold = cacheConf.heapUsageThreshold; + this.cacheAccessService = blockCache != null + ? CacheAccessServices.fromBlockCache(blockCache) + : CacheAccessServices.disabled(); } private CacheConfig() { @@ -270,6 +280,7 @@ public class CacheConfig implements PropagatingConfigurationObserver { this.blockCache = null; this.byteBuffAllocator = ByteBuffAllocator.HEAP; this.heapUsageThreshold = DEFAULT_PREFETCH_HEAP_USAGE_THRESHOLD; + this.cacheAccessService = CacheAccessServices.disabled(); } /** @@ -469,6 +480,20 @@ public class CacheConfig implements PropagatingConfigurationObserver { return Optional.ofNullable(this.blockCache); } + /** + * Returns the cache access service used by HFile read/write path callers. + * <p> + * This service is the migration-facing cache abstraction. For now it is backed by the existing + * {@link BlockCache} when block cache is configured, or by a disabled no-op implementation when + * block cache is unavailable. This keeps cache construction unchanged while allowing callers such + * as {@code HFileReaderImpl} to depend on {@link CacheAccessService}. + * </p> + * @return cache access service + */ + public CacheAccessService getCacheAccessService() { + return cacheAccessService; + } + public boolean isCombinedBlockCache() { return blockCache instanceof CombinedBlockCache; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java index 72e6cc1807a..2d4a0174517 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePreadReader.java @@ -21,6 +21,7 @@ import java.io.IOException; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.io.FSDataInputStreamWrapper; +import org.apache.hadoop.hbase.io.hfile.cache.CacheAccessService; import org.apache.yetus.audience.InterfaceAudience; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -65,7 +66,7 @@ public class HFilePreadReader extends HFileReaderImpl { // Don't use BlockIterator here, because it's designed to read load-on-open section. long onDiskSizeOfNextBlock = -1; // if we are here, block cache is present anyways - BlockCache cache = cacheConf.getBlockCache().get(); + CacheAccessService cacheAccessService = cacheConf.getCacheAccessService(); boolean interrupted = false; int blockCount = 0; int dataBlockCount = 0; @@ -78,10 +79,10 @@ public class HFilePreadReader extends HFileReaderImpl { // update the offset and move on to the next block without actually going read all // the way to the cache. BlockCacheKey cacheKey = new BlockCacheKey(name, offset); - if (cache.isAlreadyCached(cacheKey).orElse(false)) { + if (cacheAccessService.isAlreadyCached(cacheKey).orElse(false)) { // Right now, isAlreadyCached is only supported by BucketCache, which should // always cache data blocks. - int size = cache.getBlockSize(cacheKey).orElse(0); + int size = cacheAccessService.getBlockSize(cacheKey).orElse(0); if (size > 0) { offset += size; LOG.debug("Found block of size {} for cache key {}. " @@ -108,11 +109,12 @@ public class HFilePreadReader extends HFileReaderImpl { /* cacheBlock= */true, /* pread= */false, false, false, null, null, true); try { if (!cacheConf.isInMemory()) { - if (!cache.blockFitsIntoTheCache(block).orElse(true)) { + if (!cacheAccessService.blockFitsIntoTheCache(block).orElse(true)) { LOG.warn( "Interrupting prefetch for file {} because block {} of size {} " + "doesn't fit in the available cache space. isCacheEnabled: {}", - path, cacheKey, block.getOnDiskSizeWithHeader(), cache.isCacheEnabled()); + path, cacheKey, block.getOnDiskSizeWithHeader(), + cacheAccessService.isCacheEnabled()); interrupted = true; break; } @@ -139,8 +141,8 @@ public class HFilePreadReader extends HFileReaderImpl { } } if (!interrupted) { - cacheConf.getBlockCache().get().notifyFileCachingCompleted(path, blockCount, - dataBlockCount, offset); + cacheAccessService.notifyFileCachingCompleted(path, blockCount, dataBlockCount, + offset); } } catch (IOException e) { // IOExceptions are probably due to region closes (relocation, etc.) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java index 6e8f11711ae..6e09e0ba345 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java @@ -47,6 +47,7 @@ import org.apache.hadoop.hbase.io.compress.Compression; import org.apache.hadoop.hbase.io.encoding.DataBlockEncoder; import org.apache.hadoop.hbase.io.encoding.DataBlockEncoding; import org.apache.hadoop.hbase.io.encoding.HFileBlockDecodingContext; +import org.apache.hadoop.hbase.io.hfile.cache.CacheAccessService; import org.apache.hadoop.hbase.monitoring.ThreadLocalServerSideScanMetrics; import org.apache.hadoop.hbase.nio.ByteBuff; import org.apache.hadoop.hbase.regionserver.KeyValueScanner; @@ -1104,120 +1105,120 @@ public abstract class HFileReaderImpl implements HFile.Reader, Configurable { boolean updateCacheMetrics, BlockType expectedBlockType, DataBlockEncoding expectedDataBlockEncoding) throws IOException { // Check cache for block. If found return. - BlockCache cache = cacheConf.getBlockCache().orElse(null); + CacheAccessService cacheAccessService = cacheConf.getCacheAccessService(); long cachedBlockBytesRead = 0; - if (cache != null) { - HFileBlock cachedBlock = null; - boolean isScanMetricsEnabled = ThreadLocalServerSideScanMetrics.isScanMetricsEnabled(); - try { - cachedBlock = (HFileBlock) cache.getBlock(cacheKey, cacheBlock, useLock, updateCacheMetrics, - expectedBlockType); - if (cachedBlock != null) { - if (cacheConf.shouldCacheCompressed(cachedBlock.getBlockType().getCategory())) { - HFileBlock compressedBlock = cachedBlock; - cachedBlock = compressedBlock.unpack(hfileContext, fsBlockReader); - // In case of compressed block after unpacking we can release the compressed block - if (compressedBlock != cachedBlock) { - compressedBlock.release(); - } - } - try { - validateBlockType(cachedBlock, expectedBlockType); - } catch (IOException e) { - returnAndEvictBlock(cache, cacheKey, cachedBlock); - cachedBlock = null; - throw e; + HFileBlock cachedBlock = null; + boolean isScanMetricsEnabled = ThreadLocalServerSideScanMetrics.isScanMetricsEnabled(); + try { + cachedBlock = (HFileBlock) cacheAccessService.getBlock(cacheKey, cacheBlock, useLock, + updateCacheMetrics, expectedBlockType); + if (cachedBlock != null) { + if (cacheConf.shouldCacheCompressed(cachedBlock.getBlockType().getCategory())) { + HFileBlock compressedBlock = cachedBlock; + cachedBlock = compressedBlock.unpack(hfileContext, fsBlockReader); + // In case of compressed block after unpacking we can release the compressed block + if (compressedBlock != cachedBlock) { + compressedBlock.release(); } + } + try { + validateBlockType(cachedBlock, expectedBlockType); + } catch (IOException e) { + returnAndEvictBlock(cacheAccessService, cacheKey, cachedBlock); + cachedBlock = null; + throw e; + } - if (expectedDataBlockEncoding == null) { - return cachedBlock; - } - DataBlockEncoding actualDataBlockEncoding = cachedBlock.getDataBlockEncoding(); - // Block types other than data blocks always have - // DataBlockEncoding.NONE. To avoid false negative cache misses, only - // perform this check if cached block is a data block. + if (expectedDataBlockEncoding == null) { + return cachedBlock; + } + DataBlockEncoding actualDataBlockEncoding = cachedBlock.getDataBlockEncoding(); + // Block types other than data blocks always have + // DataBlockEncoding.NONE. To avoid false negative cache misses, only + // perform this check if cached block is a data block. + if ( + cachedBlock.getBlockType().isData() + && !actualDataBlockEncoding.equals(expectedDataBlockEncoding) + ) { + // This mismatch may happen if a Scanner, which is used for say a + // compaction, tries to read an encoded block from the block cache. + // The reverse might happen when an EncodedScanner tries to read + // un-encoded blocks which were cached earlier. + // + // Because returning a data block with an implicit BlockType mismatch + // will cause the requesting scanner to throw a disk read should be + // forced here. This will potentially cause a significant number of + // cache misses, so update so we should keep track of this as it might + // justify the work on a CompoundScanner. if ( - cachedBlock.getBlockType().isData() - && !actualDataBlockEncoding.equals(expectedDataBlockEncoding) + !expectedDataBlockEncoding.equals(DataBlockEncoding.NONE) + && !actualDataBlockEncoding.equals(DataBlockEncoding.NONE) ) { - // This mismatch may happen if a Scanner, which is used for say a - // compaction, tries to read an encoded block from the block cache. - // The reverse might happen when an EncodedScanner tries to read - // un-encoded blocks which were cached earlier. - // - // Because returning a data block with an implicit BlockType mismatch - // will cause the requesting scanner to throw a disk read should be - // forced here. This will potentially cause a significant number of - // cache misses, so update so we should keep track of this as it might - // justify the work on a CompoundScanner. - if ( - !expectedDataBlockEncoding.equals(DataBlockEncoding.NONE) - && !actualDataBlockEncoding.equals(DataBlockEncoding.NONE) - ) { - // If the block is encoded but the encoding does not match the - // expected encoding it is likely the encoding was changed but the - // block was not yet evicted. Evictions on file close happen async - // so blocks with the old encoding still linger in cache for some - // period of time. This event should be rare as it only happens on - // schema definition change. - LOG.info( - "Evicting cached block with key {} because data block encoding mismatch; " - + "expected {}, actual {}, path={}", - cacheKey, actualDataBlockEncoding, expectedDataBlockEncoding, path); - // This is an error scenario. so here we need to release the block. - returnAndEvictBlock(cache, cacheKey, cachedBlock); - } - cachedBlock = null; - return null; + // If the block is encoded but the encoding does not match the + // expected encoding it is likely the encoding was changed but the + // block was not yet evicted. Evictions on file close happen async + // so blocks with the old encoding still linger in cache for some + // period of time. This event should be rare as it only happens on + // schema definition change. + LOG.info( + "Evicting cached block with key {} because data block encoding mismatch; " + + "expected {}, actual {}, path={}", + cacheKey, actualDataBlockEncoding, expectedDataBlockEncoding, path); + // This is an error scenario. so here we need to release the block. + returnAndEvictBlock(cacheAccessService, cacheKey, cachedBlock); } - return cachedBlock; - } - } catch (Exception e) { - if (cachedBlock != null) { - returnAndEvictBlock(cache, cacheKey, cachedBlock); + cachedBlock = null; + return null; } - LOG.warn("Failed retrieving block from cache with key {}. " - + "\n Evicting this block from cache and will read it from file system. " - + "\n Exception details: ", cacheKey, e); - if (LOG.isDebugEnabled()) { - LOG.debug("Further tracing details for failed block cache retrieval:" + return cachedBlock; + } + } catch (Exception e) { + if (cachedBlock != null) { + returnAndEvictBlock(cacheAccessService, cacheKey, cachedBlock); + } + LOG.warn("Failed retrieving block from cache with key {}. " + + "\n Evicting this block from cache and will read it from file system. " + + "\n Exception details: ", cacheKey, e); + if (LOG.isDebugEnabled()) { + LOG.debug( + "Further tracing details for failed block cache retrieval:" + "\n Complete File path - {}," + "\n Expected Block Type - {}, Actual Block Type - {}," + "\n Cache compressed - {}" + "\n Header size (after deserialized from cache) - {}" + "\n Size with header - {}" + "\n Uncompressed size without header - {} " - + "\n Total byte buffer size - {}" + "\n Encoding code - {}", this.path, - expectedBlockType, (cachedBlock != null ? cachedBlock.getBlockType() : "N/A"), - (expectedBlockType != null - ? cacheConf.shouldCacheCompressed(expectedBlockType.getCategory()) - : "N/A"), - (cachedBlock != null ? cachedBlock.headerSize() : "N/A"), - (cachedBlock != null ? cachedBlock.getOnDiskSizeWithHeader() : "N/A"), - (cachedBlock != null ? cachedBlock.getUncompressedSizeWithoutHeader() : "N/A"), - (cachedBlock != null ? cachedBlock.getBufferReadOnly().limit() : "N/A"), - (cachedBlock != null - ? cachedBlock.getBufferReadOnly().getShort(cachedBlock.headerSize()) - : "N/A")); - } - return null; - } finally { - // Count bytes read as cached block is being returned - if (isScanMetricsEnabled && cachedBlock != null) { - cachedBlockBytesRead = cachedBlock.getOnDiskSizeWithHeader(); - // Account for the header size of the next block if it exists - if (cachedBlock.getNextBlockOnDiskSize() > 0) { - cachedBlockBytesRead += cachedBlock.headerSize(); - } - } - if (cachedBlockBytesRead > 0) { - ThreadLocalServerSideScanMetrics.addBytesReadFromBlockCache(cachedBlockBytesRead); + + "\n Total byte buffer size - {}" + "\n Encoding code - {}", + this.path, expectedBlockType, (cachedBlock != null ? cachedBlock.getBlockType() : "N/A"), + (expectedBlockType != null + ? cacheConf.shouldCacheCompressed(expectedBlockType.getCategory()) + : "N/A"), + (cachedBlock != null ? cachedBlock.headerSize() : "N/A"), + (cachedBlock != null ? cachedBlock.getOnDiskSizeWithHeader() : "N/A"), + (cachedBlock != null ? cachedBlock.getUncompressedSizeWithoutHeader() : "N/A"), + (cachedBlock != null ? cachedBlock.getBufferReadOnly().limit() : "N/A"), + (cachedBlock != null + ? cachedBlock.getBufferReadOnly().getShort(cachedBlock.headerSize()) + : "N/A")); + } + return null; + } finally { + // Count bytes read as cached block is being returned + if (isScanMetricsEnabled && cachedBlock != null) { + cachedBlockBytesRead = cachedBlock.getOnDiskSizeWithHeader(); + // Account for the header size of the next block if it exists + if (cachedBlock.getNextBlockOnDiskSize() > 0) { + cachedBlockBytesRead += cachedBlock.headerSize(); } } + if (cachedBlockBytesRead > 0) { + ThreadLocalServerSideScanMetrics.addBytesReadFromBlockCache(cachedBlockBytesRead); + } } return null; } - private void returnAndEvictBlock(BlockCache cache, BlockCacheKey cacheKey, Cacheable block) { + private void returnAndEvictBlock(CacheAccessService cacheAccessService, BlockCacheKey cacheKey, + Cacheable block) { block.release(); - cache.evictBlock(cacheKey); + cacheAccessService.evictBlock(cacheKey); } /** @@ -1373,9 +1374,8 @@ public abstract class HFileReaderImpl implements HFile.Reader, Configurable { // type in the cache key, and we expect it to match on a cache hit. if (cachedBlock.getDataBlockEncoding() != dataBlockEncoder.getDataBlockEncoding()) { // Remember to release the block when in exceptional path. - cacheConf.getBlockCache().ifPresent(cache -> { - returnAndEvictBlock(cache, cacheKey, cachedBlock); - }); + CacheAccessService cacheAccessService = cacheConf.getCacheAccessService(); + returnAndEvictBlock(cacheAccessService, cacheKey, cachedBlock); throw new IOException("Cached block under key " + cacheKey + " " + "has wrong encoding: " + cachedBlock.getDataBlockEncoding() + " (expected: " + dataBlockEncoder.getDataBlockEncoding() + "), path=" + path); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/BlockCacheBackedCacheAccessService.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/BlockCacheBackedCacheAccessService.java index 4b3d0ef6566..32c012c96f8 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/BlockCacheBackedCacheAccessService.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/BlockCacheBackedCacheAccessService.java @@ -20,6 +20,7 @@ package org.apache.hadoop.hbase.io.hfile.cache; import java.util.Objects; import java.util.Optional; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.io.hfile.BlockCache; import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; import org.apache.hadoop.hbase.io.hfile.BlockType; @@ -297,4 +298,11 @@ public class BlockCacheBackedCacheAccessService implements CacheAccessService { Objects.requireNonNull(config, "config must not be null"); blockCache.onConfigurationChange(config); } + + @Override + public void notifyFileCachingCompleted(Path fileName, int totalBlockCount, int dataBlockCount, + long size) { + Objects.requireNonNull(fileName, "fileName must not be null"); + blockCache.notifyFileCachingCompleted(fileName, totalBlockCount, dataBlockCount, size); + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessService.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessService.java index ab258eabc14..b4660a71dbf 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessService.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessService.java @@ -19,6 +19,7 @@ package org.apache.hadoop.hbase.io.hfile.cache; import java.util.Optional; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; import org.apache.hadoop.hbase.io.hfile.BlockType; import org.apache.hadoop.hbase.io.hfile.CacheStats; @@ -420,4 +421,27 @@ public interface CacheAccessService { default void onConfigurationChange(Configuration config) { // noop } + + /** + * Notifies the cache service that cache population for an HFile has completed. + * <p> + * This callback is used for file-level cache lifecycle notifications. Some cache implementations + * may use it to finalize file-scoped metadata, update fully-cached-file tracking, publish cache + * population statistics, or trigger implementation-specific bookkeeping after a writer/prefetcher + * has finished caching blocks for an HFile. + * </p> + * <p> + * The default implementation is a no-op because not all cache implementations need + * file-completion notifications. Callers may invoke this method unconditionally after file-level + * cache population completes. + * </p> + * @param fileName path of the HFile whose cache population completed + * @param totalBlockCount total number of cached blocks for the file + * @param dataBlockCount number of cached data blocks for the file + * @param size total cached size for the file, in bytes + */ + default void notifyFileCachingCompleted(Path fileName, int totalBlockCount, int dataBlockCount, + long size) { + // noop + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheEngine.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheEngine.java index 8d6629de3e2..b5564657dfb 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheEngine.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheEngine.java @@ -19,6 +19,7 @@ package org.apache.hadoop.hbase.io.hfile.cache; import java.util.Optional; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; import org.apache.hadoop.hbase.io.hfile.BlockType; import org.apache.hadoop.hbase.io.hfile.CacheStats; @@ -289,4 +290,16 @@ public interface CacheEngine { default void onConfigurationChange(Configuration config) { // noop } + + /** + * Notifies the cache service that cache population for an HFile has completed. + * @param fileName path of the HFile whose cache population completed + * @param totalBlockCount total number of cached blocks for the file + * @param dataBlockCount number of cached data blocks for the file + * @param size total cached size for the file, in bytes + */ + default void notifyFileCachingCompleted(Path fileName, int totalBlockCount, int dataBlockCount, + long size) { + // noop + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheTopology.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheTopology.java index daf7a053ea4..16e13914873 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheTopology.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheTopology.java @@ -198,4 +198,5 @@ public interface CacheTopology { * @return read-only topology view */ CacheTopologyView getView(); + } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/NoOpCacheAccessService.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/NoOpCacheAccessService.java index 570ab93e8b2..5c9da57aa10 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/NoOpCacheAccessService.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/NoOpCacheAccessService.java @@ -20,6 +20,7 @@ package org.apache.hadoop.hbase.io.hfile.cache; import java.util.Objects; import java.util.Optional; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; import org.apache.hadoop.hbase.io.hfile.CacheStats; import org.apache.hadoop.hbase.io.hfile.Cacheable; @@ -283,4 +284,10 @@ public final class NoOpCacheAccessService implements CacheAccessService { public void onConfigurationChange(Configuration config) { Objects.requireNonNull(config, "config must not be null"); } + + @Override + public void notifyFileCachingCompleted(Path fileName, int totalBlockCount, int dataBlockCount, + long size) { + Objects.requireNonNull(fileName, "fileName must not be null"); + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java index c85ed803f11..eea755218b6 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java @@ -17,8 +17,10 @@ */ package org.apache.hadoop.hbase.io.hfile; +import static org.junit.Assert.assertSame; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -37,6 +39,9 @@ import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; import org.apache.hadoop.hbase.io.ByteBuffAllocator; import org.apache.hadoop.hbase.io.hfile.BlockType.BlockCategory; import org.apache.hadoop.hbase.io.hfile.bucket.BucketCache; +import org.apache.hadoop.hbase.io.hfile.cache.BlockCacheBackedCacheAccessService; +import org.apache.hadoop.hbase.io.hfile.cache.CacheAccessService; +import org.apache.hadoop.hbase.io.hfile.cache.NoOpCacheAccessService; import org.apache.hadoop.hbase.io.util.MemorySizeUtil; import org.apache.hadoop.hbase.nio.ByteBuff; import org.apache.hadoop.hbase.testclassification.IOTests; @@ -362,8 +367,9 @@ public class TestCacheConfig { } }); // The eviction thread in lrublockcache needs to run. - while (initialL1BlockCount != lbc.getBlockCount()) + while (initialL1BlockCount != lbc.getBlockCount()) { Threads.sleep(10); + } assertEquals(initialL1BlockCount, lbc.getBlockCount()); } @@ -417,4 +423,28 @@ public class TestCacheConfig { onHeapCacheSize = MemorySizeUtil.getOnHeapCacheSize(copyConf); assertEquals(fixedSize, onHeapCacheSize); } + + @Test + void testCacheAccessServiceBackedByBlockCacheWhenBlockCacheIsConfigured() { + Configuration conf = this.conf; + BlockCache blockCache = BlockCacheFactory.createBlockCache(conf); + CacheConfig cacheConfig = new CacheConfig(conf, blockCache); + + CacheAccessService service = cacheConfig.getCacheAccessService(); + + assertInstanceOf(BlockCacheBackedCacheAccessService.class, service); + assertSame(blockCache, ((BlockCacheBackedCacheAccessService) service).getBlockCache()); + } + + @Test + void testCacheAccessServiceIsNoOpWhenBlockCacheIsNull() { + Configuration conf = this.conf; + + CacheConfig cacheConfig = new CacheConfig(conf, null); + + CacheAccessService service = cacheConfig.getCacheAccessService(); + + assertInstanceOf(NoOpCacheAccessService.class, service); + assertFalse(service.isCacheEnabled()); + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestBlockCacheBackedCacheAccessService.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestBlockCacheBackedCacheAccessService.java index bce743aa1b5..55dd5a579db 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestBlockCacheBackedCacheAccessService.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestBlockCacheBackedCacheAccessService.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.io.hfile.cache; +import static org.junit.Assert.assertThrows; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; @@ -29,6 +30,7 @@ import static org.mockito.Mockito.when; import java.util.Optional; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.io.hfile.BlockCache; import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; import org.apache.hadoop.hbase.io.hfile.BlockType; @@ -218,4 +220,27 @@ public class TestBlockCacheBackedCacheAccessService { service.onConfigurationChange(new Configuration(false)); service.shutdown(); } + + @Test + void testNotifyFileCachingCompletedDelegatesToBlockCache() { + BlockCache blockCache = mock(BlockCache.class); + CacheAccessService service = new BlockCacheBackedCacheAccessService(blockCache); + Path fileName = new Path("/hbase/table/region/family/file"); + int totalBlockCount = 10; + int dataBlockCount = 8; + long size = 1024L; + + service.notifyFileCachingCompleted(fileName, totalBlockCount, dataBlockCount, size); + + verify(blockCache).notifyFileCachingCompleted(fileName, totalBlockCount, dataBlockCount, size); + } + + @Test + void testNotifyFileCachingCompletedRejectsNullPath() { + BlockCache blockCache = mock(BlockCache.class); + CacheAccessService service = new BlockCacheBackedCacheAccessService(blockCache); + + assertThrows(NullPointerException.class, + () -> service.notifyFileCachingCompleted(null, 10, 8, 1024L)); + } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestNoOpCacheAccessService.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestNoOpCacheAccessService.java new file mode 100644 index 00000000000..582aa7e0241 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestNoOpCacheAccessService.java @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.io.hfile.cache; + +import static org.junit.Assert.assertThrows; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.Mockito.mock; + +import java.util.Optional; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; +import org.apache.hadoop.hbase.io.hfile.CacheStats; +import org.apache.hadoop.hbase.io.hfile.Cacheable; +import org.apache.hadoop.hbase.io.hfile.HFileBlock; +import org.apache.hadoop.hbase.testclassification.IOTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Tests for {@link NoOpCacheAccessService} and related service helpers. + */ +@Tag(IOTests.TAG) +@Tag(SmallTests.TAG) + +public class TestNoOpCacheAccessService { + + private static final String HFILE_NAME = "file"; + private static final String REGION_NAME = "region"; + private static final long BLOCK_OFFSET = 1L; + private static final long RANGE_START_OFFSET = 1L; + private static final long RANGE_END_OFFSET = 10L; + + @Test + void testNoOpCacheAccessService() { + CacheAccessService service = new NoOpCacheAccessService(new CacheStats("noop")); + BlockCacheKey key = new BlockCacheKey(HFILE_NAME, BLOCK_OFFSET); + CacheRequestContext requestContext = CacheRequestContext.newBuilder().setCaching(true) + .setRepeat(false).setUpdateCacheMetrics(true).build(); + CacheWriteContext writeContext = CacheWriteContext.newBuilder().setInMemory(true) + .setWaitWhenCache(true).setSource(CacheWriteSource.READ_MISS).build(); + Cacheable block = mock(Cacheable.class); + Configuration conf = new Configuration(false); + + assertEquals("NoOpCacheAccessService", service.getName()); + assertNull(service.getBlock(key, requestContext)); + + service.cacheBlock(key, block, writeContext); + + assertFalse(service.evictBlock(key)); + assertEquals(0, service.evictBlocksByHfileName(HFILE_NAME)); + assertEquals(0, + service.evictBlocksRangeByHfileName(HFILE_NAME, RANGE_START_OFFSET, RANGE_END_OFFSET)); + assertEquals(0, service.evictBlocksByRegionName(REGION_NAME)); + + assertEquals(0L, service.getMaxSize()); + assertEquals(0L, service.getFreeSize()); + assertEquals(0L, service.size()); + assertEquals(0L, service.getCurrentDataSize()); + assertEquals(0L, service.getBlockCount()); + assertEquals(0L, service.getDataBlockCount()); + + assertEquals(Optional.empty(), service.blockFitsIntoTheCache(mock(HFileBlock.class))); + assertEquals(Optional.empty(), service.isAlreadyCached(key)); + assertEquals(Optional.empty(), service.getBlockSize(key)); + + assertFalse(service.isCacheEnabled()); + assertFalse(service.waitForCacheInitialization(1L)); + + service.onConfigurationChange(conf); + service.shutdown(); + } + + @Test + void testNoOpCacheAccessServiceRejectsNullInputs() { + CacheAccessService service = new NoOpCacheAccessService(new CacheStats("noop")); + Cacheable block = mock(Cacheable.class); + CacheRequestContext requestContext = CacheRequestContext.newBuilder().build(); + CacheWriteContext writeContext = CacheWriteContext.newBuilder().build(); + + assertThrows(NullPointerException.class, () -> new NoOpCacheAccessService(null)); + assertThrows(NullPointerException.class, () -> service.getBlock(null, requestContext)); + assertThrows(NullPointerException.class, + () -> service.getBlock(new BlockCacheKey(HFILE_NAME, BLOCK_OFFSET), null)); + assertThrows(NullPointerException.class, () -> service.cacheBlock(null, block, writeContext)); + assertThrows(NullPointerException.class, + () -> service.cacheBlock(new BlockCacheKey(HFILE_NAME, BLOCK_OFFSET), null, writeContext)); + assertThrows(NullPointerException.class, + () -> service.cacheBlock(new BlockCacheKey(HFILE_NAME, BLOCK_OFFSET), block, null)); + assertThrows(NullPointerException.class, () -> service.evictBlock(null)); + assertThrows(NullPointerException.class, () -> service.evictBlocksByHfileName(null)); + assertThrows(NullPointerException.class, + () -> service.evictBlocksRangeByHfileName(null, RANGE_START_OFFSET, RANGE_END_OFFSET)); + assertThrows(NullPointerException.class, () -> service.evictBlocksByRegionName(null)); + assertThrows(NullPointerException.class, () -> service.blockFitsIntoTheCache(null)); + assertThrows(NullPointerException.class, () -> service.isAlreadyCached(null)); + assertThrows(NullPointerException.class, () -> service.getBlockSize(null)); + assertThrows(NullPointerException.class, () -> service.onConfigurationChange(null)); + } +}
