wchevreuil commented on code in PR #5492:
URL: https://github.com/apache/hbase/pull/5492#discussion_r1415985529
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/bucket/BucketCache.java:
##########
@@ -2144,4 +2146,21 @@ public Optional<Integer> getBlockSize(BlockCacheKey key)
{
}
}
+
+ public Optional<Map<String, Integer>>
+ uncacheStaleBlocks(RegionAvailabilityChecker regionAvailabilityChecker) {
+ Map<String, Integer> evictedFilesWithStaleBlocks = new HashMap<>();
Review Comment:
Copying my previous concerns shared on the other PR:
> Hum, I think we should make this method async. And we shouldn't block the
RPC call until its finished. We should dedicate a thread pool with a max size
of one.
I'm worried now this might not be a lightweight operation, if the related
RPC is synchronous (as implemented here), we might eventually timeout and
client might submit another call, eventually exhausting RPC handlers.
Simply making the RPC async isn't enough, we should also make sure we don't
have more than one background thread running this.`
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/CombinedBlockCache.java:
##########
@@ -439,6 +441,16 @@ public Optional<Map<String, Pair<String, Long>>>
getFullyCachedFiles() {
return this.l2Cache.getFullyCachedFiles();
}
+ @Override
+ public Optional<Map<String, Integer>>
+ uncacheStaleBlocks(RegionAvailabilityChecker regionAvailabilityChecker) {
+ Map<String, Integer> uncachedStaleBlocksMap =
+
l1Cache.uncacheStaleBlocks(regionAvailabilityChecker).orElseGet(HashMap::new);
+ l2Cache.uncacheStaleBlocks(regionAvailabilityChecker).ifPresent(
+ map2 -> map2.forEach((key, value) -> uncachedStaleBlocksMap.merge(key,
value, Integer::sum)));
Review Comment:
Why merge? We should just do putAll, no?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java:
##########
@@ -245,4 +246,15 @@ default Optional<Integer> getBlockSize(BlockCacheKey key) {
default Optional<Map<String, Pair<String, Long>>> getFullyCachedFiles() {
return Optional.empty();
}
+
+ /**
+ * Clean Cache by evicting the blocks of files belonging to regions that are
no longer served by
+ * the RegionServer.
+ * @param regionAvailabilityChecker RegionAvailabilityChecker
+ * @return A map of filename and number of blocks evicted.
+ */
+ default Optional<Map<String, Integer>>
+ uncacheStaleBlocks(RegionAvailabilityChecker regionAvailabilityChecker) {
Review Comment:
I think @Apache9 meant to use the `RegionServerServices` interface in the
method signature, not creating a new interface.
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
