VladRodionov commented on code in PR #8653:
URL: https://github.com/apache/hbase/pull/8653#discussion_r4067823486
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/CacheAccessServices.java:
##########
@@ -81,33 +77,59 @@ public static CacheAccessService fromBlockCache(BlockCache
blockCache) {
DefaultHBaseCachePlacementAdmissionPolicy.INSTANCE);
}
+ /**
+ * Creates a {@link CacheAccessService} from the block cache configuration.
+ * @param conf cache configuration
+ * @return configured cache access service, or a disabled service when block
caching is disabled
+ * @throws NullPointerException if {@code conf} is {@code null}
+ */
+ public static CacheAccessService fromConfiguration(Configuration conf) {
+ return fromConfiguration(conf, null);
+ }
+
/**
* Creates a {@link CacheAccessService} from the block cache configuration.
* <p>
- * This method is a compatibility factory for tests and transitional code
paths that want to
- * obtain a {@link CacheAccessService} directly from {@link Configuration},
while still using the
- * existing {@link BlockCacheFactory} and legacy {@link BlockCache}
implementations underneath.
- * </p>
- * <p>
- * The method delegates block-cache construction to
- * {@link BlockCacheFactory#createBlockCache(Configuration)}. If the legacy
factory creates a
- * {@link BlockCache}, the returned service is backed by that cache through
- * {@link TopologyBackedCacheAccessService}. If the legacy factory does not
create a cache, this
- * method returns the disabled/no-op cache access service.
- * </p>
- * <p>
- * This method does not introduce new cache-engine or topology-based runtime
wiring. It is
- * intended only as a bridge while existing HBase tests and integration
paths migrate from direct
- * {@link BlockCache} usage to {@link CacheAccessService}.
+ * Cache implementations that implement {@link CacheEngine} natively are
used directly. Legacy
+ * {@link BlockCache} implementations are adapted to {@link CacheEngine}
until their migration is
+ * complete.
* </p>
- * @param conf configuration used by {@link BlockCacheFactory}
- * @return cache access service created from the configured legacy block
cache, or disabled when
- * no block cache is configured
+ * @param conf cache configuration
+ * @param onlineRegions currently online regions, or {@code null} when
unavailable
+ * @return configured cache access service, or a disabled service when block
caching is disabled
* @throws NullPointerException if {@code conf} is {@code null}
*/
- public static CacheAccessService fromConfiguration(Configuration conf) {
+ public static CacheAccessService fromConfiguration(Configuration conf,
+ Map<String, HRegion> onlineRegions) {
Objects.requireNonNull(conf, "conf must not be null");
- return fromBlockCache(BlockCacheFactory.createBlockCache(conf));
+
+ CacheEngine l1 = BlockCacheFactory.createFirstLevelCacheEngine(conf);
+ if (l1 == null) {
+ return disabled();
+ }
+
+ CachePlacementAdmissionPolicy policy =
DefaultHBaseCachePlacementAdmissionPolicy.INSTANCE;
+
+ boolean useExternal =
conf.getBoolean(BlockCacheFactory.EXTERNAL_BLOCKCACHE_KEY,
+ BlockCacheFactory.EXTERNAL_BLOCKCACHE_DEFAULT);
+
+ if (useExternal) {
+ CacheEngine l2 = BlockCacheFactory.createExternalCacheEngine(conf);
+ if (l2 == null) {
+ return
TopologyBackedCacheAccessServices.fromSingleCacheEngine("single", l1, policy);
+ }
+
+ return
TopologyBackedCacheAccessServices.fromTieredInclusiveCacheEngines("inclusive",
l1, l2,
+ policy);
+ }
+
+ CacheEngine l2 = BlockCacheFactory.createBucketCacheEngine(conf,
onlineRegions);
+ if (l2 == null) {
+ return TopologyBackedCacheAccessServices.fromSingleCacheEngine("single",
l1, policy);
+ }
+
+ return
TopologyBackedCacheAccessServices.fromTieredExclusiveCacheEngines("combined",
l1, l2,
+ policy);
Review Comment:
The tiered topologies currently expose only L1 statistics, so L2 activity is
indeed missing from the service-level view. I’ll add a topology-level aggregate
CacheStats view for the participating engines rather than returning
l1.getStats().
While reviewing this, I also found that the exclusive lookup path currently
updates the selected engine's metrics twice: once through
CacheEngine.getBlock(..., updateCacheMetrics=true) and again through
updateBlockMetrics(). I’ll remove the redundant manual update so the aggregate
view does not double-count exclusive-cache requests. Inclusive lookup metrics
will retain the existing per-tier behavior, consistent with the legacy
inclusive combined-cache 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]
