VladRodionov commented on code in PR #8575:
URL: https://github.com/apache/hbase/pull/8575#discussion_r3890654516
##########
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 single-tier block caches through TopologyBackedCacheAccessService
changed the
meaning of getCurrentSize() because the topology-backed implementation
returned current data size
instead of delegating the legacy BlockCache#getCurrentSize() value.
I fixed this by adding CacheEngine#getCurrentSize(), delegating it in
BlockCacheBackedCacheEngine,
and aggregating it in TopologyBackedCacheAccessService. I also added a
single-tier compatibility
test that verifies getCurrentSize() and getCurrentDataSize() remain distinct.
--
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]
