HBASE-30104 Refactor LruBlockCache to implement CacheEngine - #8653
VladRodionov wants to merge 1 commit into
Conversation
713b0df to
b729865
Compare
|
The failure is not related to the PR. Its ready for review. cc: @taklwu |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Native LRU replacement, eviction concurrency, tier handling, and statistics accounting have unresolved correctness issues.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (7)
What changed in this PR
Introduces a native LRU cache engine and integrates it with the pluggable cache topology architecture.
Changes:
- Adds native LRU caching and eviction-listener support.
- Moves tier eviction handling into cache topologies.
- Updates cache construction, compatibility behavior, and tests.
| File | Description |
|---|---|
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java | Validates native LRU use in HFile tests. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestForceCacheImportantBlocks.java | Retains legacy cache setup for this test. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheOnWrite.java | Uses a legacy-backed cache service. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java | Updates topology and eviction tests. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestCacheEngineConstruction.java | Tests native LRU construction. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServiceTestFactory.java | Adds tier engine lookup support. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java | Exposes cloning and encoding helpers. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CacheConfig.java | Preserves native services when copied. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessServices.java | Adds native-engine topology factories. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessService.java | Connects engine eviction events to topologies. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TieredInclusiveTopology.java | Handles inclusive eviction events. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TieredExclusiveTopology.java | Demotes L1 evictions to L2. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/SingleTierTopology.java | Defines single-tier eviction behavior. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/LruCacheEngine.java | Implements the native LRU engine. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheTopology.java | Adds topology eviction handling. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheEvictionListener.java | Defines capacity-eviction callbacks. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheEngine.java | Adds eviction-listener registration. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServices.java | Constructs native cache topologies. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheUtil.java | Adds native-engine replacement checks. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheFactory.java | Creates native and adapted cache engines. |
| dev-support/spotbugs-exclude.xml | Excludes the intentional constructor-started thread. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
My first impression on Copilot review - AI started hallucinating, but maybe I am not correct, I will dig deeper into these review. |
b729865 to
a965cfa
Compare
| * </p> | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class LruCacheEngine implements CacheEngine, HeapSize, Iterable<CachedBlock> { |
There was a problem hiding this comment.
LruBlockCache and LruCacheEngine are very similar, what is the plan for LruBlockCache in the future?
There was a problem hiding this comment.
All block caches as well as BlockCache interface will be retired. The reason why some of them seem so similar is because they have to, with some future exceptions, such as BucketCache (BucketCacheEngine will contains core of BucketCache less orchestration/placement logic), TinyLFUBlockCache and AdaptiveLRUBlockCache will be gone completely and replaced by corresponding implementations of CacheAdmissionPlacementPolicy and this policies can be used with any types of CacheEngine giving them new functionality.
| LOG.info("Allocating CacheEngine size=" + StringUtils.byteDesc(cacheSize) + ", blockSize=" | ||
| + StringUtils.byteDesc(blockSize)); | ||
|
|
||
| if (policy.equalsIgnoreCase("LRU")) { |
There was a problem hiding this comment.
should we add other supported policy in BlockCacheFactory or other constants class? I found those have been inline plaintext in few other classes as well.
| if (policy.equalsIgnoreCase("LRU")) { | |
| if (policy.equalsIgnoreCase(BlockCacheFactory.BLOCKCACHE_POLICY_DEFAULT)) { |
There was a problem hiding this comment.
I will address this once start working on Factory.
I normally combine my own view + claude or other cursor model to verify them, and it could be nonsense if you know better than it. |
This PR introduces LruCacheEngine as the native CacheEngine implementation of the existing
LruBlockCache functionality.
The goal is to migrate the LRU block cache implementation to the new pluggable block cache
architecture without requiring LruBlockCache to be wrapped by BlockCacheBackedCacheEngine.
Main changes:
configuration, and test-facing functionality.
responsibility of CacheTopology.
evicted blocks to L2.
reinsert a block already present in L2.
implementations directly where available.
have not yet been migrated.
legacy BlockCache is available.
underlying LruBlockCache.
The production RegionServer still owns a legacy BlockCache. Migrating RegionServer ownership to
CacheAccessService is intentionally left for a follow-up issue. Dynamic cache sizing used by
HeapMemoryManager is also being handled separately.
Tests: