Repository navigation
HBASE-30363 Migrate AdaptiveLRU to LruCacheEngine and adaptive policy - #8766
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open findings
L1(e.g., a tiered topology whereL2is temporarily… · NewonEvictionPressure(..., 0L)when the eviction lock cannot be acquired is misleading:… · NewcurrentSizeis0(which is likely on a fresh engine), making… · New
What changed in this PR
Migrates AdaptiveLRU in the CacheAccessService path from LruAdaptiveBlockCache to native LruCacheEngine with a dedicated adaptive placement/admission policy, and wires eviction-pressure feedback from engines through topology to the policy.
Changes:
- Adds
AdaptiveLruCachePlacementAdmissionPolicyand routes eviction-pressure callbacks to it viaCacheEvictionListenerandTopologyBackedCacheAccessService. - Updates
LruCacheEngineto report “requested eviction pressure” (bytesToFree) to listeners. - Adds/updates tests to validate pressure reporting/routing and AdaptiveLRU behavior across topologies and construction paths.
| File | Description |
|---|---|
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestTopologyBackedCacheAccessService.java | Adds tests ensuring eviction-pressure goes to the policy and block evictions still go to topology. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestLruCacheEngine.java | Adds tests for eviction-pressure reporting from LruCacheEngine. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestAdaptiveLruCachePlacementAdmissionPolicy.java | Adds unit tests for AdaptiveLRU admission adjustment and topology behaviors. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestCacheConfig.java | Adds tests asserting the CacheAccessService path uses LruCacheEngine + adaptive policy, while legacy BlockCache path remains LruAdaptiveBlockCache. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessService.java | Creates tier-bound eviction listeners to route block evictions to topology and pressure to policy. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/LruCacheEngine.java | Implements eviction-pressure notification on eviction pass completion. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CachePlacementAdmissionPolicy.java | Adds a default onEvictionPressure(...) callback to the policy interface. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheEvictionListener.java | Adds a default onEvictionPressure(...) listener callback and updates documentation. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServices.java | Instantiates the adaptive policy when AdaptiveLRU is configured. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/AdaptiveLruCachePlacementAdmissionPolicy.java | New AdaptiveLRU logic moved into a policy, controlling L1 admission/placement. |
| hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCacheFactory.java | Switches AdaptiveLRU CacheEngine creation to native LruCacheEngine for the engine-based path. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
| * @param bytesToFree amount of cache pressure observed, in bytes | ||
| */ | ||
| @Override | ||
| public void onEvictionPressure(CacheEngine sourceEngine, long bytesToFree) { |
There was a problem hiding this comment.
CacheEvictionListener#onEvictionPressure has parameter with sourceEngine that is not being used when creating, but the tier is passing to the policy.onEvictionPressure
why do we have this mis-match ? should we always use CacheEvictionListener with tier?
There was a problem hiding this comment.
CacheEvictionListener is an engine-level API, so it reports the CacheEngine that generated the event. The engine does not know whether it is used as SINGLE, L1, or L2.
TopologyBackedCacheAccessService knows that mapping, so when it registers the listener it captures the corresponding CacheTier and passes that tier to the policy.
So the mismatch is intentional: engine event in, topology tier out. I would keep CacheEvictionListener independent of CacheTier.
Remove notifyEvictionPressure call when eviction lock is not acquired Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
17b8736 to
41dd3f1
Compare
|
Thanks, @taklwu as usual :) |
Summary
Migrates the native CacheAccessService path for AdaptiveLRU from
LruAdaptiveBlockCachetoLruCacheEngineplus a dedicated adaptiveplacement/admission policy.
The legacy
BlockCacheconstruction path remains unchanged.Changes
AdaptiveLruCachePlacementAdmissionPolicy.CacheEvictionListener.CachePlacementAdmissionPolicy.LruCacheEnginereport requested eviction pressure (bytesToFree).TopologyBackedCacheAccessServiceroute:CacheTopology;CachePlacementAdmissionPolicy.LruCacheEnginefor AdaptiveLRU in the CacheAccessService path.LruAdaptiveBlockCachefor the legacyBlockCacheFactoryconstruction path.
AdaptiveLRU behavior
The adaptive algorithm preserves the behavior of the legacy
LruAdaptiveBlockCache, including:For tiered caches, AdaptiveLRU controls L1 admission only. It does not
globally reject a block that can still be placed in L2.
Tests
Adds coverage for:
LruCacheEngine;TopologyBackedCacheAccessService;LruAdaptiveBlockCacheconstruction path.