VladRodionov commented on code in PR #8575:
URL: https://github.com/apache/hbase/pull/8575#discussion_r3890627907
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessServices.java:
##########
@@ -110,6 +111,155 @@ public static TopologyBackedCacheAccessService
fromTieredExclusiveBlockCaches(St
return new TopologyBackedCacheAccessService(topology, policy);
}
+ /**
+ * Creates a topology-backed cache access service for an {@link
InclusiveCombinedBlockCache}.
+ * <p>
+ * {@link InclusiveCombinedBlockCache} represents a two-tier inclusive cache
layout. Unlike the
+ * exclusive {@link CombinedBlockCache} path, a block may be present in more
than one tier. The
+ * resulting service therefore uses {@link TieredInclusiveTopology}, not
+ * {@link TieredExclusiveTopology}.
+ * </p>
+ * <p>
+ * The supplied legacy combined cache is used only as a source of the
existing first-level and
+ * second-level block caches. Each tier is wrapped in a {@link
BlockCacheBackedCacheEngine}, and
+ * the new {@link TopologyBackedCacheAccessService} performs access through
the topology
+ * abstraction.
+ * </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 tiers
+ */
+ public static TopologyBackedCacheAccessService
+ fromInclusiveCombinedBlockCache(InclusiveCombinedBlockCache
combinedBlockCache) {
+ Objects.requireNonNull(combinedBlockCache, "combinedBlockCache must not be
null");
+
+ BlockCache[] blockCaches = combinedBlockCache.getBlockCaches();
+ if (blockCaches.length != 2) {
+ throw new IllegalArgumentException(
+ "InclusiveCombinedBlockCache must expose exactly two block caches");
+ }
+
+ return fromTieredInclusiveBlockCaches("inclusive-combined",
blockCaches[0], blockCaches[1],
+ DefaultHBaseCachePlacementAdmissionPolicy.INSTANCE);
Review Comment:
Good catch. The issue is not just validation here. A real
InclusiveCombinedBlockCache has already
wired L1 to L2 through the victim-cache mechanism, so using the raw L1 as an
independent topology
engine can leak L2 lookup through the L1 engine.
I fixed this by adding a non-victim-delegating first-level cache engine
adapter. The adapter checks
L1 membership with FirstLevelBlockCache.containsBlock(...) before calling
getBlock(...). If the key
is not present in L1, it returns null and lets the topology-backed service
perform the L2 lookup
explicitly.
I also updated the tiered topology factories to use this adapter for L1
engines and added a test
that constructs a real InclusiveCombinedBlockCache to verify that an L1 miss
does not internally
delegate to L2 through the victim path.
--
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]
