VladRodionov commented on code in PR #8279:
URL: https://github.com/apache/hbase/pull/8279#discussion_r3320405880
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java:
##########
@@ -1104,120 +1105,120 @@ public HFileBlock getCachedBlock(BlockCacheKey
cacheKey, boolean cacheBlock, boo
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;
Review Comment:
Thanks. I think I would avoid the explicit `instanceof
NoOpCacheAccessService` check here.
`CacheAccessService` is intended to be the abstraction boundary, so
`HFileReaderImpl` should not need
to know which concrete service implementation it received. The
disabled-cache behavior belongs in
`NoOpCacheAccessService`; its `getBlock(...)` returns null, so the current
flow already exits
without doing cache work.
If there is concern about future bugs in `NoOpCacheAccessService`, I think
the better protection is
unit coverage for that implementation rather than adding
implementation-specific checks in the read
path. This keeps the reader code independent of whether the service is
block-cache-backed,
topology-backed, or no-op.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
