VladRodionov commented on code in PR #8697:
URL: https://github.com/apache/hbase/pull/8697#discussion_r4099208005
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/cache/TieredInclusiveTopology.java:
##########
@@ -133,4 +133,20 @@ public void shutdown() {
public boolean handleEviction(BlockCacheKey cacheKey, Cacheable block,
CacheEngine sourceEngine) {
return false;
}
+
+ /**
+ * Handles an access to a block found in this inclusive topology.
+ * <p>
+ * A block found in L1 is also expected to be present in L2. Notify L2 about
the access so that it
+ * can update any access-based metadata without reading the block. An L2 hit
requires no
+ * additional notification because L2 observed the access directly.
+ * @param cacheKey key identifying the accessed block
+ * @param sourceEngine engine in which the block was found
+ */
+ @Override
+ public void handleAccess(BlockCacheKey cacheKey, CacheEngine sourceEngine) {
+ if (sourceEngine == l1) {
+ l2.touch(cacheKey);
Review Comment:
The observation that the currently adapted legacy L2 does not implement a
meaningful touch() is correct, but I don't think HBASE-30406 should emulate it
through BlockCacheBackedCacheEngine.
touch() is intentionally an optional CacheEngine operation.
TieredInclusiveTopology defines the topology-level semantic that an L1 access
is propagated to L2, while individual engines decide whether and how they can
process that notification.
In particular, legacy BlockCache has no metadata-only touch API.
Implementing BlockCacheBackedCacheEngine.touch() by issuing another getBlock()
would add an L2 lookup to every L1 hit and would also introduce
statistics/reference-counting side effects, which is exactly what this
notification API is intended to avoid.
This adapter is transitional anyway. BlockCacheBackedCacheEngine exists to
bridge legacy BlockCache implementations into the new architecture and is
expected to be retired once the remaining block caches are converted to native
CacheEngine implementations.
The next native engine planned for this work, BucketCacheEngine, will have a
non-default touch() implementation because BucketCache has access metadata that
can be updated without performing another cache lookup. That is the kind of
engine-specific implementation this API is intended to support.
Likewise, adding meaningful touch behavior to other native engines is
engine-specific functionality and does not need to be coupled to the
introduction of the topology notification contract itself.
I therefore prefer to keep the inclusive topology notification in place and
leave unsupported engines as no-ops. This establishes the topology contract
now, while allowing native engines to implement efficient access notification
as they are introduced. I can also clarify the JavaDoc/PR description so it
does not imply that all currently adapted engines update their access metadata
through touch().
--
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]
