Copilot commented on code in PR #8766: URL: https://github.com/apache/hbase/pull/8766#discussion_r4276594223
########## hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/AdaptiveLruCachePlacementAdmissionPolicy.java: ########## @@ -0,0 +1,306 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.io.hfile.cache; + +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; +import org.apache.hadoop.hbase.io.hfile.Cacheable; +import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Placement and admission policy implementing AdaptiveLRU behavior using a native + * {@link LruCacheEngine}. + * <p> + * The policy preserves the adaptive admission behavior of the legacy {@code LruAdaptiveBlockCache}. + * Under sustained LRU eviction pressure, it reduces the percentage of data blocks admitted to the + * first-level cache. When eviction pressure decreases, data-block admission is gradually restored. + * </p> + * <p> + * Non-data blocks are not filtered by the adaptive algorithm. + * </p> + */ [email protected] +public final class AdaptiveLruCachePlacementAdmissionPolicy + implements CachePlacementAdmissionPolicy { + + private static final Logger LOG = + LoggerFactory.getLogger(AdaptiveLruCachePlacementAdmissionPolicy.class); + + private static final String LRU_CACHE_HEAVY_EVICTION_COUNT_LIMIT = + "hbase.lru.cache.heavy.eviction.count.limit"; + + private static final int DEFAULT_LRU_CACHE_HEAVY_EVICTION_COUNT_LIMIT = Integer.MAX_VALUE; + + private static final String LRU_CACHE_HEAVY_EVICTION_MB_SIZE_LIMIT = + "hbase.lru.cache.heavy.eviction.mb.size.limit"; + + private static final long DEFAULT_LRU_CACHE_HEAVY_EVICTION_MB_SIZE_LIMIT = 500L; + + private static final String LRU_CACHE_HEAVY_EVICTION_OVERHEAD_COEFFICIENT = + "hbase.lru.cache.heavy.eviction.overhead.coefficient"; + + private static final float DEFAULT_LRU_CACHE_HEAVY_EVICTION_OVERHEAD_COEFFICIENT = 0.01f; + + private static final long EVICTION_EVALUATION_PERIOD_MS = 10_000L; + + private static final int MIN_CACHE_DATA_BLOCK_PERCENT = 1; + + private static final int MAX_CACHE_DATA_BLOCK_PERCENT = 100; + + private static final int MAX_ADMISSION_REDUCTION_PER_PERIOD = 15; + + private static final CachePlacementAdmissionPolicy DELEGATE = + DefaultHBaseCachePlacementAdmissionPolicy.INSTANCE; + + private final int heavyEvictionCountLimit; + + private final long heavyEvictionMbSizeLimit; + + private final float heavyEvictionOverheadCoefficient; + + private volatile int cacheDataBlockPercent = MAX_CACHE_DATA_BLOCK_PERCENT; + + private long freedSumMb; + + private int heavyEvictionCount; + + private long evaluationStartTime; + + /** + * Creates an AdaptiveLRU placement/admission policy from configuration. + * @param conf cache configuration + * @throws NullPointerException if {@code conf} is {@code null} + */ + public AdaptiveLruCachePlacementAdmissionPolicy(Configuration conf) { + Objects.requireNonNull(conf, "conf must not be null"); + + this.heavyEvictionCountLimit = Math.max(conf.getInt(LRU_CACHE_HEAVY_EVICTION_COUNT_LIMIT, + DEFAULT_LRU_CACHE_HEAVY_EVICTION_COUNT_LIMIT), 0); + + this.heavyEvictionMbSizeLimit = Math.max(conf.getLong(LRU_CACHE_HEAVY_EVICTION_MB_SIZE_LIMIT, + DEFAULT_LRU_CACHE_HEAVY_EVICTION_MB_SIZE_LIMIT), 1L); + + float configuredCoefficient = conf.getFloat(LRU_CACHE_HEAVY_EVICTION_OVERHEAD_COEFFICIENT, + DEFAULT_LRU_CACHE_HEAVY_EVICTION_OVERHEAD_COEFFICIENT); + configuredCoefficient = Math.min(configuredCoefficient, 1.0f); + configuredCoefficient = Math.max(configuredCoefficient, 0.001f); + this.heavyEvictionOverheadCoefficient = configuredCoefficient; + + this.evaluationStartTime = EnvironmentEdgeManager.currentTime(); + } + + /** + * Decides whether a block should be admitted into the cache. + * <p> + * In a single-tier topology, adaptive rejection means the data block is not cached. In a tiered + * topology, admission remains enabled because adaptive filtering applies specifically to the L1 + * placement decision rather than to the entire cache hierarchy. + * </p> + * @param cacheKey block cache key + * @param block block to cache + * @param context cache write context + * @param priority admission priority + * @param topologyView read-only topology view + * @return admission decision + */ + @Override + public AdmissionDecision shouldAdmit(BlockCacheKey cacheKey, Cacheable block, + CacheWriteContext context, AdmissionPriority priority, CacheTopologyView topologyView) { + Objects.requireNonNull(cacheKey, "cacheKey must not be null"); + Objects.requireNonNull(block, "block must not be null"); + Objects.requireNonNull(context, "context must not be null"); + Objects.requireNonNull(priority, "priority must not be null"); + Objects.requireNonNull(topologyView, "topologyView must not be null"); + + AdmissionDecision decision = + DELEGATE.shouldAdmit(cacheKey, block, context, priority, topologyView); + if (!decision.isAdmitted()) { + return decision; + } + + if ( + topologyView.getType() == CacheTopologyType.SINGLE_TIER + && !shouldCacheInFirstLevel(cacheKey, block) + ) { + return AdmissionDecision.reject("Rejected by AdaptiveLRU data-block admission"); + } + + return decision; + } + + /** + * Selects the target tier or tiers for an admitted block. + * <p> + * The default HBase placement decision is preserved unless AdaptiveLRU rejects a data block from + * L1. In a tiered topology this removes L1 from the target set while retaining other selected + * tiers such as L2. + * </p> + * @param cacheKey block cache key + * @param block block to cache + * @param context cache write context + * @param topologyView read-only topology view + * @return cache tier decision + */ + @Override + public TierDecision selectTier(BlockCacheKey cacheKey, Cacheable block, CacheWriteContext context, + CacheTopologyView topologyView) { + Objects.requireNonNull(cacheKey, "cacheKey must not be null"); + Objects.requireNonNull(block, "block must not be null"); + Objects.requireNonNull(context, "context must not be null"); + Objects.requireNonNull(topologyView, "topologyView must not be null"); + + TierDecision decision = DELEGATE.selectTier(cacheKey, block, context, topologyView); + + if (!decision.getTiers().contains(CacheTier.L1) || shouldCacheInFirstLevel(cacheKey, block)) { + return decision; + } + + List<CacheTier> tiers = new ArrayList<>(); + for (CacheTier tier : decision.getTiers()) { + if (tier != CacheTier.L1) { + tiers.add(tier); + } + } + Review Comment: If the delegate placement returns only `L1` (e.g., a tiered topology where `L2` is temporarily unavailable), removing `L1` yields an empty tier list and returns `TierDecision.multiple(empty)`. That can produce an invalid placement (admitted block with nowhere to go) or trigger downstream errors depending on `TierDecision` invariants. Handle the empty-result case explicitly (e.g., return a 'no tier' decision if supported, or treat this situation like single-tier and reject admission/placement). ########## hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestLruCacheEngine.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.io.hfile.cache; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.testclassification.IOTests; +import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Tests for {@link LruCacheEngine}. + */ +@Tag(IOTests.TAG) +@Tag(SmallTests.TAG) +public class TestLruCacheEngine { + + private static final String LRU_MIN_FACTOR_CONFIG_NAME = "hbase.lru.blockcache.min.factor"; + + /** + * Verifies that an eviction pass with no excess occupancy reports zero cache pressure. + */ + @Test + void testEvictionPassReportsZeroPressureWhenBelowTargetSize() { + LruCacheEngine engine = new LruCacheEngine(1024L * 1024L, 64L * 1024L, false); + CacheEvictionListener listener = mock(CacheEvictionListener.class); + + try { + engine.setEvictionListener(listener); + + engine.evict(); + + verify(listener).onEvictionPressure(engine, 0L); + } finally { + engine.shutdown(); + } + } + + /** + * Verifies that an eviction pass reports the number of bytes the engine determines should be + * freed to reach its configured minimum target size. + */ + @Test + void testEvictionPassReportsRequestedPressure() { + Configuration conf = new Configuration(false); + conf.setFloat(LRU_MIN_FACTOR_CONFIG_NAME, 0.5f); + + LruCacheEngine engine = new LruCacheEngine(16L * 1024L * 1024L, 64L * 1024L, false, conf); + CacheEvictionListener listener = mock(CacheEvictionListener.class); + + try { + long currentSize = engine.getCurrentSize(); + long newMaxSize = Math.max(1L, currentSize / 2L); + long expectedBytesToFree = currentSize - (long) Math.floor(newMaxSize * 0.5f); + + engine.setEvictionListener(listener); + engine.setMaxSize(newMaxSize); + + verify(listener).onEvictionPressure(engine, expectedBytesToFree); Review Comment: This test can become a no-op if `currentSize` is `0` (which is likely on a fresh engine), making `expectedBytesToFree` equal to `0` and not actually validating positive-pressure reporting. To meaningfully cover 'requested pressure', preload the engine so `currentSize > minSize()` (or otherwise force an over-target condition) before reducing `maxSize`, and then assert that `expectedBytesToFree` is > 0 and is what gets reported. ########## hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/cache/TestTopologyBackedCacheAccessService.java: ########## @@ -627,6 +628,61 @@ public void testRestorePersistentState() throws IOException { verify(persistentEngine).restore(input); verify(input).close(); + } + Review Comment: These lines include trailing whitespace. Please remove trailing spaces to keep diffs clean and avoid style-check noise. ########## hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/LruCacheEngine.java: ########## @@ -801,21 +801,26 @@ long getOverhead() { */ void evict() { if (!evictionLock.tryLock()) { + notifyEvictionPressure(0L); Review Comment: Reporting `onEvictionPressure(..., 0L)` when the eviction lock cannot be acquired is misleading: the eviction pass did not run, and actual pressure may be non-zero. This can cause the adaptive policy to incorrectly interpret contention as 'no pressure' and recover admission too aggressively. Consider either (a) not notifying at all when `tryLock()` fails, or (b) computing and reporting a best-effort pressure estimate (e.g., from `size.get()` and `minSize()`) while clearly treating it as an estimate. -- 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]
