HBASE-30196 Add diagnostic cached-block iteration capability for plug…#8384
Open
VladRodionov wants to merge 1 commit into
Open
HBASE-30196 Add diagnostic cached-block iteration capability for plug…#8384VladRodionov wants to merge 1 commit into
VladRodionov wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a transitional diagnostic bridge for cached-block iteration while the HBase block cache API migrates from direct BlockCache usage toward CacheAccessService. It updates several tests to use CacheAccessService while preserving the ability to inspect cached blocks for diagnostic assertions.
Changes:
- Make
BlockCacheBackedCacheAccessServiceexpose cached-block iteration by delegating to the backingBlockCacheiterator. - Add/adjust
CacheConfigconstructors and test helpers to supportCacheAccessService-based test wiring. - Update affected tests to use
CacheAccessServicewhile keeping legacy diagnostic/cache-inspection behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSecureBulkLoadManager.java | Update test CacheConfig construction to use CacheAccessService-aware constructor signature. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestRowIndexV1RoundTrip.java | Update CacheConfig construction for CacheAccessService signature. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java | Switch test wiring from BlockCache to CacheAccessService via test factory. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileReaderImpl.java | Mock CacheAccessService instead of BlockCache for corrupt-cache test path. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java | Update tests to create/access caches through CacheAccessService test factory and unwrap legacy cache when needed. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java | Use CacheAccessService in test setup and stats checks; unwrap legacy BlockCache for region creation. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java | Convert cache-on-write tests to CacheAccessService while retaining cached-block inspection logic. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java | Update cache config tests to use CacheAccessService test factory and the new APIs. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServiceTestFactory.java | Add helper to unwrap the legacy BlockCache from a CacheAccessService for compatibility tests. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAvoidCellReferencesIntoShippedBlocks.java | Update test to obtain cached-block iteration via CacheAccessServices helper (but currently contains a runtime type bug). |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java | Add constructors supporting CacheAccessService and centralize config initialization. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessService.java | Add getCurrentSize() and adjust size-related documentation/behavior (currently inconsistent). |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/NoOpCacheAccessService.java | Implement getCurrentSize() for the disabled cache service. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServices.java | Add helper for optional cached-block iteration discovery via CacheAccessService. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessService.java | Extend ConfigurationObserver and add getCurrentSize() API. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/BlockCacheBackedCacheAccessService.java | Implement Iterable<CachedBlock> and delegate iteration and getCurrentSize() to the backing BlockCache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+418
to
+419
| iterator = (Iterator<CachedBlock>) CacheAccessServices.asCachedBlockIterable(cache) | ||
| .orElseThrow(); |
taklwu
reviewed
Jun 22, 2026
| * @param conf hbase configuration | ||
| * @param blockCache block cache to use for this configuration | ||
| */ | ||
| public CacheConfig(Configuration conf, BlockCache blockCache) { |
Contributor
There was a problem hiding this comment.
with the new public CacheConfig(Configuration conf, CacheAccessService service) { , will we migrate or remove this constructor in the future ?
Contributor
Author
There was a problem hiding this comment.
This is the test only constructor, which mirrors (Configuration, BlockCache) counterpart. The old one will be removed. Removing (Configuration conf, CacheAccessService service) will require some extensive refactoring of a CacheConfig class. Not sure if CacheConfig refactoring will be a part of this work.
Comment on lines
+418
to
+419
| iterator = (Iterator<CachedBlock>) CacheAccessServices.asCachedBlockIterable(cache) | ||
| .orElseThrow(); |
…gable cache engines
VladRodionov
force-pushed
the
HBASE-30196-cache-diagnostic-iteration
branch
from
dce868a to
cffd012
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds a diagnostic/test bridge for cached-block iteration while the block cache architecture is migrating from direct BlockCache usage toward CacheAccessService.
The existing BlockCache interface extends Iterable, and a number of tests and diagnostic paths rely on that capability. CacheAccessService should remain the normal read/write-path facade and should not become a full replacement for every BlockCache method. However, tests and diagnostics still need a way to access the legacy cached-block iterator during the transition.
Changes
Compatibility notes
This is a transition/diagnostic capability. It does not change cache read/write semantics.
The main runtime path still uses CacheAccessService for cache access. Cached-block iteration remains intended for tests, diagnostics, and admin-style inspection only. Future CacheEngine/topology-backed implementations can expose equivalent diagnostic iteration when available.
Out of scope
Testing
Ran:
mvn -pl hbase-server test mvn -pl hbase-server -DskipTests compile mvn -pl hbase-server spotless:check mvn -pl hbase-server checkstyle:checkDisclosure
Parts of this PR, including helper code structure and PR description text, were prepared with assistance from ChatGPT and reviewed by the author.