Copilot commented on code in PR #8575:
URL: https://github.com/apache/hbase/pull/8575#discussion_r3899400781
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessServices.java:
##########
@@ -104,12 +105,210 @@ public static TopologyBackedCacheAccessService
fromTieredExclusiveBlockCaches(St
Objects.requireNonNull(l2, "l2 must not be null");
Objects.requireNonNull(policy, "policy must not be null");
wireVictimCache(l1, l2);
- CacheEngine l1Engine = CacheEngines.fromBlockCache(l1);
+ CacheEngine l1Engine = fromL1BlockCache(l1);
CacheEngine l2Engine = CacheEngines.fromBlockCache(l2);
CacheTopology topology = new TieredExclusiveTopology(name, l1Engine,
l2Engine);
return new TopologyBackedCacheAccessService(topology, policy);
}
+ /**
+ * Creates a topology-backed cache access service for an {@link
InclusiveCombinedBlockCache}.
+ * <p>
+ * The inclusive combined cache must expose exactly two non-null legacy
block caches. The first
+ * cache is adapted as L1 using a non-victim-delegating engine, and the
second cache is adapted as
+ * L2. This prevents L1 misses from internally consulting L2 through the
legacy victim-cache
+ * mechanism and lets the topology-backed service control tier lookup and
promotion policy.
+ * </p>
+ * @param combinedBlockCache inclusive combined block cache to adapt
+ * @return topology-backed cache access service using a tiered inclusive
topology
+ * @throws NullPointerException if {@code combinedBlockCache} is {@code
null}
+ * @throws IllegalArgumentException if the combined cache does not expose
exactly two non-null
+ * block caches
+ */
+ public static TopologyBackedCacheAccessService
+ fromInclusiveCombinedBlockCache(InclusiveCombinedBlockCache
combinedBlockCache) {
+ Objects.requireNonNull(combinedBlockCache, "combinedBlockCache must not be
null");
+
+ BlockCache[] blockCaches = combinedBlockCache.getBlockCaches();
+ if (blockCaches == null || blockCaches.length != 2) {
+ throw new IllegalArgumentException(
+ "InclusiveCombinedBlockCache must expose exactly two block caches");
+ }
+ if (blockCaches[0] == null || blockCaches[1] == null) {
+ throw new IllegalArgumentException(
+ "InclusiveCombinedBlockCache must expose non-null L1 and L2 block
caches");
+ }
+
+ return fromTieredInclusiveBlockCaches("inclusive-combined",
blockCaches[0], blockCaches[1],
+ DefaultHBaseCachePlacementAdmissionPolicy.INSTANCE);
Review Comment:
This factory does not preserve inclusive insertion semantics.
`DefaultHBaseCachePlacementAdmissionPolicy.selectTier` chooses only L1 for
metadata or L2 for data, and the service writes only to the selected tiers,
whereas `InclusiveCombinedBlockCache.cacheBlock` writes every block to both
caches. Use an inclusive compatibility policy (or return both tiers for
`TIERED_INCLUSIVE`) and test this actual factory path.
##########
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:
The `containsBlock` precheck is not a safe local-only lookup. It skips the
wrapped cache's miss accounting when false; if the entry is concurrently
evicted after returning true, `super.getBlock` sees a miss and can still follow
the wired victim cache, misattributing an L2 hit as L1. Use an atomic
local-only retrieval path that preserves the original `repeat`/metrics
semantics.
This issue also appears on line 106 of the same file.
--
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]
