HBASE-30471 Remove LruBlockCache dependencies from test and production code by VladRodionov · Pull Request #8844 · apache/hbase

VladRodionov · 2026-11-04T23:45:16Z

HBASE-30471 Remove LruBlockCache dependencies from test and production code

Summary

This PR continues the pluggable block cache migration by removing direct dependencies on
LruBlockCache from production and test code where the new CacheEngine /
CacheAccessService APIs can be used instead.

The main goal is to move LRU-specific behavior and tests to LruCacheEngine while keeping legacy
BlockCache implementations supported through the existing compatibility adapters.

This PR is based on HBASE-30018 and assumes the HBASE-30383 RegionServer
CacheAccessService migration is already present in that branch.

Changes

Migrates the former TestLruBlockCache coverage to TestLruCacheEngine.
Removes TestLruBlockCache.
Extends LruCacheEngine with the functionality needed by the migrated unit tests.
Updates HFile/cache production code to use CacheEngine / CacheAccessService abstractions
instead of depending directly on LruBlockCache.
Updates affected HFile and cache tests to work through the new cache abstractions.
Adds BlockCacheTestUtils for test cases that still need to exercise legacy BlockCache
implementations during the transition.
Extends CacheTestUtils, CacheAccessServiceTestFactory, and
CacheAccessServiceTestInstance to support both legacy BlockCache and new CacheEngine
based tests.
Updates TopologyBackedCacheAccessService and related helpers where needed to expose the
functionality required by migrated callers and tests.
Keeps compatibility with legacy cache implementations through the existing adapter layer rather
than introducing new direct LruBlockCache dependencies.

Scope

This PR does not attempt to remove every remaining BlockCache reference from HBase.

BlockCache is still required for legacy cache implementations and compatibility adapters that have
not yet been migrated. Those references will be removed incrementally as the remaining cache
implementations are converted.

The purpose of this change is specifically to eliminate unnecessary LruBlockCache dependencies
from code that can now use the pluggable cache APIs.

Behavior

No user-visible cache behavior change is intended.

The changes are primarily an internal API and test migration from the concrete
LruBlockCache implementation to LruCacheEngine, CacheEngine, and
CacheAccessService.

Validation

The affected cache and HFile unit tests are exercised as part of this change, including the migrated
TestLruCacheEngine coverage and tests for cache access, cache-on-write, HFile block handling,
scanner behavior, and block encoding.

git diff --check is clean.

AI-assisted development

This change was developed in collaboration with ChatGPT 5.6 Sol. ChatGPT was used as a
development and review assistant for code migration, test adaptation, API design discussion, and
review feedback. All resulting code and design decisions were reviewed and validated by the author.

VladRodionov · 2026-11-05T04:26:01Z

Failed test is flaky. PR is ready for review @anmolnar @taklwu

VladRodionov · 2026-11-10T00:35:01Z

Ran PR through GPT Astra.

No actionable findings in PR #8844 at commit 897814a.
Reviewed all 23 changed files and relevant callers. Validation passed on Java 21: 208 tests across six focused suites, with no failures or skips, covering LRU behavior, cache factories, compression, encoding, scanner reference counts, and cache-on-write.
The full HBase test suite was not run.

Copilot

🟡 Changes recommended

The test inspects a cache the region never uses, so its zero-count assertions pass even if scanner selection reads excluded files.

1 open issue

Use the region's cache to verify scanner selection · New

What changed in this PR

This PR continues HBase’s pluggable block cache migration by replacing direct LruBlockCache dependencies with LruCacheEngine and cache-service abstractions, while retaining legacy compatibility adapters. No user-visible behavior change is intended.

Changes:

Moves LRU-specific coverage into TestLruCacheEngine.
Migrates HFile tests and cache inspection to CacheAccessService.
Extends test helpers and updates production API documentation and compatibility handling.

File	Description
`hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/EncodedSeekPerformanceTest.java`	Clears cached blocks through the service API.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingKeyRange.java`	Uses service-based cached-file inspection.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java`	Removes the migrated legacy test suite.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLazyDataBlockDecompression.java`	Tests compression with native LRU engines.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileScannerImplReferenceCount.java`	Migrates LRU scanner coverage to the service API.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileDataBlockEncoder.java`	Uses a service-backed LRU test instance.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockIndex.java`	Creates caches through service test helpers.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCombinedBlockCache.java`	Uses the extracted concurrency test helper.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java`	Uses service-based configuration and cache clearing.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/CacheTestUtils.java`	Adds engine overloads for shared assertions.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestLruCacheEngine.java`	Receives migrated LRU behavior coverage.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestCacheAccessServiceTestFactory.java`	Verifies native LRU factory instances.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServiceTestInstance.java`	Supports native engines and legacy caches.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServiceTestFactory.java`	Creates native LRU services and single-tier wrappers.
`hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/BlockCacheTestUtils.java`	Extracts concurrent cache-operation helpers.
`hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruCachedBlock.java`	Updates the documented owning cache implementation.
`hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java`	Generalizes heap-allocation comments.
`hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java`	Generalizes heap-allocation parameter documentation.
`hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessServices.java`	Returns null when no legacy cache backs an engine.
`hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessService.java`	Updates cache-size documentation to reference native LRU.
`hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/LruCacheEngine.java`	Exposes configuration and eviction-thread access for tests.
`hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheEngine.java`	Documents native engines and compatibility adapters.
`hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessService.java`	Updates documentation to reference `LruCacheEngine`.

_{Review effort: Balanced}


    Scan scan = new Scan().withStartRow(Bytes.toBytes("aaa")).withStopRow(Bytes.toBytes("aaz"));
-    BlockCache cache = BlockCacheFactory.createBlockCache(conf);
+    CacheAccessService cache = CacheAccessServices.fromConfiguration(conf);


anmolnar

AI-assisted review — drafted by Claude (Anthropic) on behalf of @anmolnar

Thanks for the migration, this is in good shape overall. I went through all 23 files. Two things I verified up front:

All 13 tests from the deleted TestLruBlockCache are present in TestLruCacheEngine with identical bodies (only the class name changes, and the multi-thread helper moved to BlockCacheTestUtils).
The Copilot finding on TestScannerSelectionUsingKeyRange looks addressed in c6d4b7f: the CacheAccessService is now created before the region and passed to createRegionAndWAL, and the file-count assertion is no longer guarded by instanceof LruBlockCache. That thread can be resolved.

A few questions and requests before I approve:

1. TopologyBackedCacheAccessServices.getBlockCache now returns null
The IllegalArgumentException for a non-BlockCacheBackedCacheEngine was replaced with return null, but the only two callers touched by this patch (both in TestCacheOnWrite) were removed. Which caller needs the new behaviour? It also makes the method inconsistent: a missing tier still throws, while a native engine returns null. Could we either keep the exception, or return Optional<BlockCache> and update the Javadoc?

2. CacheAccessServiceTestInstance.blockCache() contract change
It used to return the concrete T and now returns BlockCache, which is null for native engines. Since every lruInstance(...) now produces an LruCacheEngine, any remaining test on the branch that calls lruInstance(...).blockCache() would get null at runtime instead of a compile error. Could you confirm there are no such callers left? Dropping the T extends BlockCache bound in favour of a runtime instanceof check also loses some compile-time safety; two small factory methods (one per kind) might be cleaner.

3. EncodedSeekPerformanceTest: Lists import
This imports org.apache.hadoop.util.Lists, which is a Hadoop-internal class. Please use org.apache.hbase.thirdparty.com.google.common.collect.Lists (as TestCacheOnWrite does) or new ArrayList<>(...).

4. TestCacheOnWrite: possibly unused import
Both usages of TopologyBackedCacheAccessServices visible in the diff were removed, but the import remains. If nothing else in the file uses it, checkstyle will complain.

5. Widened visibility in LruCacheEngine
getEvictionThread() and LRU_HARD_CAPACITY_LIMIT_FACTOR_CONFIG_NAME are now public so tests in io.hfile can reach them. Fine for the constant; for getEvictionThread() could we add @RestrictedApi limited to test paths?

Nits (non-blocking)

CacheAccessServiceTestFactory.singleTierService has no Javadoc, is unused in this patch, and duplicates the private createCacheAccessService in CacheAccessServiceTestInstance.
CacheEngine Javadoc: the edit drops a </p>, leaving a paragraph unclosed.
TestHFileScannerImplReferenceCount.testDisabledBlockCache: the comment "Must be a single-tier LruCacheEngine" sits next to a null cache.
CacheTestUtils.getBlockAndAssertEquals(CacheEngine, ...): actualBlock is dereferenced before the null check in finally; an assertNotNull first would give a clearer failure.
BlockCacheTestUtils: the two overloads are identical bodies; a shared private helper would remove the duplication.
TestScannerSelectionUsingKeyRange: the new cache service is never shut down.

VladRodionov · 2026-11-11T04:13:45Z

on Claude 2. comment. Confirmed. There are no remaining callers of lruInstance(...).blockCache() after the LRU tests were migrated to LruCacheEngine.
CacheAccessServiceTestInstance is temporary test infrastructure used during the BlockCache -> CacheEngine migration. The remaining BlockCache support exists only for tests that have not yet been migrated and will be removed together with those tests/legacy APIs in the final migration.
Because of that, I would prefer not to introduce separate permanent-looking factory abstractions for the two implementations. The relaxed generic is transitional and allows the same test fixture to be used while individual tests are migrated. I did change blockCache() to fail explicitly when the instance is backed by a native CacheEngine, rather than silently returning null, so accidental use from a migrated LRU test is detected immediately.

on Clause nit in BlockCacheTestUtils : The duplication is intentional here. BlockCache and CacheEngine expose the same operations needed by this test, but they do not share a common interface for them. Extracting a shared helper would require either adapting the BlockCache to a CacheEngine (which changes what the legacy test is exercising) or introducing test-only functional/interface plumbing that I think makes this simple concurrency test less clear.
This is also transitional code: the BlockCache overload exists only for the remaining legacy tests and will be removed as those tests are migrated to CacheEngine, at which point the duplication disappears. I would prefer to keep the two implementations explicit until then.

The rest of the review was fixed.

…n code

anmolnar · 2026-11-11T16:33:29Z

Thanks @VladRodionov . I'm not sure about what has been changed exactly, I suggest to push multiple commits onto the PR in the future, rather than force pushing a single commit to get better visibility on the different changes.

Let me share Claude's comments on the latest version.

AI-assisted review — drafted by Claude (Anthropic) on behalf of @anmolnar

Thanks for the quick turnaround and the explanations. I compared 2bb2bcd against c6d4b7f and can confirm the points from my previous review are addressed.

On your two replies:

CacheAccessServiceTestInstance: agreed. Keeping a single transitional fixture makes sense given it will be removed with the legacy tests, and failing explicitly in blockCache() covers the concern I had.
BlockCacheTestUtils duplication: agreed, fine to keep the two overloads explicit until the BlockCache one is removed.

A few small things from the update itself:

1. TestCacheOnWrite.isBucketCacheL2 is unused
The new private helper has no callers, and the Optional import exists only for it. The cacheOnCompactAndNonBucketCache condition checks only CacheTier.SINGLE. Should the helper be part of that condition, or can it be removed?

2. CacheTier.SINGLE lookup in the same condition
CacheAccessServiceTestFactory.getCacheEngine(cache, CacheTier.SINGLE) throws if the tier is absent. Can you confirm none of the parameterised caches in this test is a tiered (L1/L2) topology, or handle that case?

3. @RestrictedApi import in LruCacheEngine
The diff adds the annotation without an import hunk. Fine if the file already imports it; just flagging in case.

Nits

CacheAccessServiceTestInstance.blockCache(): the Javadoc still says it returns null for a native engine, but it now throws IllegalStateException. The blockCache field is also no longer read after this change.
TestScannerSelectionUsingKeyRange: cache.shutdown() is called before closeRegionAndWAL(region); closing the region first would be safer.

Once CI is green on this commit (and with a note on which test was flaky earlier), I'm happy to approve.

anmolnar

lgtm. 🚢

VladRodionov · 2026-11-11T19:21:17Z

Yes, it is always CacheTier.SINGLE in this test
required import is in place, otherwise code won't even compile :)

Addressed the rest of a comments

VladRodionov · 2026-11-11T22:46:22Z

The failures are not PR-related. Verified locally two failed test: TestSyncReplicationStandbyKillMaster PASSES. TestMobCloneSnapshotFromClientAfterSplittingRegion is flaky - this is not a first time when it fails periodically.

VladRodionov · 2026-11-12T18:41:48Z

Thank you, @anmolnar @taklwu for the review.

taklwu requested a balanced review from Copilot November 10, 2026 02:01

Copilot started reviewing on behalf of taklwu November 10, 2026 02:02

View session

Copilot AI reviewed Nov 10, 2026

View reviewed changes

VladRodionov force-pushed the HBASE-30471-remove-lrublockcache-dependencies branch from 897814a to c6d4b7f Compare November 10, 2026 05:21

anmolnar reviewed Nov 10, 2026

View reviewed changes

HBASE-30471 Remove LruBlockCache dependencies from test and productio…

2bb2bcd

…n code

VladRodionov force-pushed the HBASE-30471-remove-lrublockcache-dependencies branch from c6d4b7f to 2bb2bcd Compare November 11, 2026 04:42

anmolnar approved these changes Nov 11, 2026

View reviewed changes

HBASE-30471 Address review comments

18de292

anmolnar merged commit e4967c9 into apache:HBASE-30018 Nov 12, 2026
8 of 15 checks passed

Navigation Menu

Repository navigation

HBASE-30471 Remove LruBlockCache dependencies from test and production code - #8844

HBASE-30471 Remove LruBlockCache dependencies from test and production code#8844
anmolnar merged 2 commits into
apache:HBASE-30018from
VladRodionov:HBASE-30471-remove-lrublockcache-dependencies

VladRodionov commented Nov 4, 2026

Uh oh!

VladRodionov commented Nov 5, 2026

Uh oh!

VladRodionov commented Nov 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

anmolnar left a comment

Uh oh!

VladRodionov commented Nov 11, 2026 •

edited

Loading

Uh oh!

anmolnar commented Nov 11, 2026 •

edited

Loading

Uh oh!

anmolnar left a comment

Uh oh!

VladRodionov commented Nov 11, 2026

Uh oh!

VladRodionov commented Nov 11, 2026

Uh oh!

Uh oh!

VladRodionov commented Nov 12, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

VladRodionov commented Nov 4, 2026