Copilot commented on code in PR #8575:
URL: https://github.com/apache/hbase/pull/8575#discussion_r3877175680
##########
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:
An actual `InclusiveCombinedBlockCache` has already wired L1's victim cache
to L2 in its constructor. Wrapping that same L1 as an independent engine means
an L1 miss calls L2 internally (and may auto-promote), so the topology reports
the hit as L1 and bypasses its L2 promotion policy; an L2 miss can also be
queried twice. The mock-based tests do not reproduce this wiring. Please use a
non-victim-delegating per-tier adapter (or otherwise remove that delegation)
and add an integration test built from a real inclusive combined cache.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TopologyBackedCacheAccessService.java:
##########
@@ -214,23 +209,79 @@ private void updateBlockMetrics(Cacheable block,
BlockCacheKey key, CacheEngine
}
/**
- * Adds a block to the cache using policy-selected target tiers.
+ * Caches a block using the topology-backed cache access service.
* <p>
- * This method first asks the configured policy whether the block should be
admitted. If admitted,
- * the policy selects the target tier or tiers. The block is then inserted
into each selected
- * engine using {@link CacheEngine#cacheBlock(BlockCacheKey, Cacheable,
boolean, boolean)}.
+ * Single-tier topology preserves the legacy direct block-cache behavior by
writing admitted
+ * blocks to the only backing engine. Tiered topologies use the placement
policy to select one or
+ * more target tiers.
* </p>
+ * @param cacheKey cache key identifying the block
+ * @param block block to cache
+ * @param context cache write context
+ * @throws NullPointerException if {@code cacheKey}, {@code block}, or
{@code context} is
+ * {@code null}
+ */
+ @Override
+ public void cacheBlock(BlockCacheKey cacheKey, Cacheable block,
CacheWriteContext context) {
+ Objects.requireNonNull(cacheKey, "cacheKey must not be null");
+ Objects.requireNonNull(block, "block must not be null");
+ Objects.requireNonNull(context, "context must not be null");
+
+ if (topology.getType() == CacheTopologyType.SINGLE_TIER) {
+ cacheBlockToSingleTier(cacheKey, block, context);
+ return;
+ }
+
+ cacheBlockToSelectedTiers(cacheKey, block, context);
+ }
+
+ /**
+ * Caches a block into the only engine in a single-tier topology.
* <p>
- * The policy's representation decision is intentionally not applied in this
initial
- * implementation. The current block object is passed through unchanged.
+ * This method preserves the behavior of the legacy {@link
BlockCacheBackedCacheAccessService}
+ * path. A single-tier topology has no placement decision to make: admitted
blocks are written to
+ * the only available engine, which is exposed as {@link CacheTier#L1}.
* </p>
- * @param cacheKey block cache key
- * @param block block contents
+ * @param cacheKey cache key identifying the block
+ * @param block block to cache
* @param context cache write context
+ * @throws NullPointerException if {@code cacheKey}, {@code block}, or
{@code context} is
+ * {@code null}
*/
+ private void cacheBlockToSingleTier(BlockCacheKey cacheKey, Cacheable block,
+ CacheWriteContext context) {
+ Objects.requireNonNull(cacheKey, "cacheKey must not be null");
+ Objects.requireNonNull(block, "block must not be null");
+ Objects.requireNonNull(context, "context must not be null");
- @Override
- public void cacheBlock(BlockCacheKey cacheKey, Cacheable block,
CacheWriteContext context) {
+ AdmissionDecision admission =
+ policy.shouldAdmit(cacheKey, block, context, AdmissionPriority.NORMAL,
topologyView);
+ if (!admission.isAdmitted()) {
+ return;
+ }
+
+ Optional<CacheEngine> engine = topology.getEngine(CacheTier.L1);
+ if (!engine.isPresent()) {
+ return;
+ }
Review Comment:
This lookup assumes every `SINGLE_TIER` topology exposes `L1`, but
`SingleEngineTopology` was changed to that same type while still exposing only
`CacheTier.SINGLE` (`SingleEngineTopology:65,91`). Consequently,
`CacheAccessServices.fromTopology(new SingleEngineTopology(...), ...)` silently
drops every admitted cache write. Resolve the sole engine through the
topology's declared tier rather than hard-coding L1.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServices.java:
##########
@@ -47,26 +48,37 @@ private CacheAccessServices() {
}
/**
- * Creates a cache access service backed by an existing block cache.
+ * Creates a {@link CacheAccessService} for the supplied legacy {@link
BlockCache}.
* <p>
- * For regular {@link BlockCache} implementations, this returns a legacy
- * {@link BlockCacheBackedCacheAccessService}. For {@link
CombinedBlockCache}, this returns a
- * topology-backed service using {@link TieredExclusiveTopology}. This moves
combined L1/L2
- * orchestration to the new topology layer while keeping the existing
combined block cache object
- * available for legacy {@link BlockCache}-facing APIs.
+ * All legacy block caches are adapted through {@link
TopologyBackedCacheAccessService}. Plain
+ * single-tier block caches are represented by {@link SingleTierTopology}.
Exclusive combined
+ * caches are represented by {@link TieredExclusiveTopology}. Inclusive
combined caches are
+ * represented by {@link TieredInclusiveTopology}.
* </p>
- * @param blockCache block cache to expose through {@link CacheAccessService}
- * @return cache access service
+ * <p>
+ * {@link InclusiveCombinedBlockCache} is checked before {@link
CombinedBlockCache} because the
+ * inclusive variant has different residency, promotion, and eviction
semantics. Routing it
+ * through the exclusive topology would be incorrect.
+ * </p>
+ * @param blockCache legacy block cache to adapt
+ * @return topology-backed cache access service for the supplied block cache
+ * @throws NullPointerException if {@code blockCache} is {@code null}
*/
-
public static CacheAccessService fromBlockCache(BlockCache blockCache) {
Objects.requireNonNull(blockCache, "blockCache must not be null");
+
+ if (blockCache instanceof InclusiveCombinedBlockCache) {
+ return TopologyBackedCacheAccessServices
+ .fromInclusiveCombinedBlockCache((InclusiveCombinedBlockCache)
blockCache);
+ }
+
if (blockCache instanceof CombinedBlockCache) {
return TopologyBackedCacheAccessServices
.fromCombinedBlockCache((CombinedBlockCache) blockCache);
}
- return new BlockCacheBackedCacheAccessService(blockCache);
+ return TopologyBackedCacheAccessServices.fromSingleBlockCache("single",
blockCache,
+ DefaultHBaseCachePlacementAdmissionPolicy.INSTANCE);
Review Comment:
Routing ordinary block caches through this service changes
`getCurrentSize()` semantics: `TopologyBackedCacheAccessService#getCurrentSize`
currently returns `getCurrentDataSize()`, whereas the old adapter delegated to
`BlockCache#getCurrentSize()`. For `LruBlockCache`, those are explicitly
different (`size` includes metadata/overhead while `dataBlockSize` does not),
so topology-backed diagnostics now under-report occupied cache memory. Please
expose/delegate `getCurrentSize` through `CacheEngine` and aggregate that value
in the topology service, with a single-tier compatibility test.
--
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]
