VladRodionov commented on code in PR #8575: URL: https://github.com/apache/hbase/pull/8575#discussion_r3900444681
########## hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/NonVictimDelegatingFirstLevelCacheEngine.java: ########## @@ -0,0 +1,129 @@ +/* + * 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.Objects; +import java.util.Optional; +import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; +import org.apache.hadoop.hbase.io.hfile.BlockType; +import org.apache.hadoop.hbase.io.hfile.Cacheable; +import org.apache.hadoop.hbase.io.hfile.FirstLevelBlockCache; +import org.apache.yetus.audience.InterfaceAudience; + +/** + * {@link CacheEngine} adapter for a {@link FirstLevelBlockCache} that suppresses victim-cache + * lookup delegation on cache misses. + * <p> + * Some legacy first-level block caches can be wired with a victim cache. In that mode, calling + * {@code getBlock(...)} on the first-level cache may internally consult the victim cache after an + * L1 miss. That behavior is correct for the legacy {@code BlockCache} composition, but it is + * incorrect when the same L1 cache is used as an independent L1 engine inside a topology-backed + * service. + * </p> + * <p> + * A topology-backed service must observe tier hits at the tier where they actually occur. If an L1 + * miss internally fetches from L2 through the victim handler, the topology would incorrectly report + * the access as an L1 hit and would bypass L2-specific promotion policy. This adapter prevents that + * by checking {@link FirstLevelBlockCache#containsBlock(BlockCacheKey)} before delegating to the + * wrapped cache. + * </p> + */ [email protected] +public class NonVictimDelegatingFirstLevelCacheEngine extends BlockCacheBackedCacheEngine { + + private final FirstLevelBlockCache firstLevelBlockCache; + + /** + * Creates a non-victim-delegating engine for a first-level block cache. + * @param firstLevelBlockCache first-level block cache to adapt + * @throws NullPointerException if {@code firstLevelBlockCache} is {@code null} + */ + public NonVictimDelegatingFirstLevelCacheEngine(FirstLevelBlockCache firstLevelBlockCache) { + super(firstLevelBlockCache); + this.firstLevelBlockCache = + Objects.requireNonNull(firstLevelBlockCache, "firstLevelBlockCache must not be null"); + } + + /** + * Returns a block only when it is actually resident in the first-level cache. + * <p> + * If the key is not present in L1, this method returns {@code null} without calling the wrapped + * cache's {@code getBlock(...)} method. This avoids invoking victim-cache lookup delegation. + * </p> + * @param cacheKey cache key identifying the block + * @param caching whether the caller intends to cache blocks during this read + * @param repeat whether this is a repeated access + * @param updateCacheMetrics whether cache metrics should be updated + * @return cached block when it is resident in L1; otherwise {@code null} + * @throws NullPointerException if {@code cacheKey} is {@code null} + */ + @Override + public Cacheable getBlock(BlockCacheKey cacheKey, boolean caching, boolean repeat, + boolean updateCacheMetrics) { + Objects.requireNonNull(cacheKey, "cacheKey must not be null"); + + if (!firstLevelBlockCache.containsBlock(cacheKey)) { + return null; + } + + return super.getBlock(cacheKey, caching, repeat, updateCacheMetrics); Review Comment: I agree with the concern, but I do not want to add a new local-only retrieval API to FirstLevelBlockCache in this PR. That would be a broader API/behavior change just to support a temporary migration bridge. The actual issue is that the topology-backed model assumes L1 and L2 are independent engines, while legacy combined-cache construction has already wired L1 to L2 through the victim-cache path. Since that legacy wiring will be removed as this migration progresses, I fixed this at the migration boundary instead. I added a temporary unsetVictimCache() method to FirstLevelBlockCache and call it when adapting L1 for tiered topology-backed services. That prevents L1.getBlock(...) from delegating to L2 internally, so the topology-backed service controls L2 lookup and promotion explicitly. I also removed the non-victim-delegating adapter class and added tests verifying that tiered topology construction clears the legacy victim-cache wiring. -- 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]
