wchevreuil commented on code in PR #5492:
URL: https://github.com/apache/hbase/pull/5492#discussion_r1380780232
##########
hbase-protocol-shaded/src/main/protobuf/server/region/Admin.proto:
##########
@@ -336,6 +336,14 @@ message ClearSlowLogResponses {
required bool is_cleaned = 1;
}
+message CleanBucketCacheRequest {
+}
+
+message CleanBucketCacheResponse {
+ repeated string uncached_files = 1;
Review Comment:
Could we return a Map<uncached_files, num_blocks_evicted> ?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java:
##########
@@ -3946,4 +3949,19 @@ public GetCachedFilesListResponse
getCachedFilesList(RpcController controller,
});
return responseBuilder.addAllCachedFiles(fullyCachedFiles).build();
}
+
+ @Override
+ public CleanBucketCacheResponse cleanBucketCache(RpcController controller,
Review Comment:
I'm confused why we are implementing the eviction logic here. The
RSRpcServices should be just a facade, finding "orphans" blocks and evict it
should be a responsibility of the cache implementation class.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/BlockCache.java:
##########
@@ -170,4 +172,8 @@ default boolean isMetaBlock(BlockType blockType) {
default Optional<Map<String, Boolean>> getFullyCachedFiles() {
return Optional.empty();
}
+
+ default List<HStoreFile> getCachedButClosedFiles() {
Review Comment:
I like the idea of exposing this info.
And let's use Optional to avoid returning null anti-pattern, just like the
getFullyCachedFiles method above.
Also the naming is not very intuitive. We could call it
getFilesWithStaleBlocks and let's put an explanatory javadoc.
Additionally, BlockCache interface should defined the clean method as well.
It's up for the BlockCache implementation to deal with the eviction logic.
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java:
##########
@@ -2634,4 +2634,9 @@ List<LogEntry> getLogEntries(Set<ServerName> serverNames,
String logType, Server
* Get the list of cached files
*/
List<String> getCachedFilesList(ServerName serverName) throws IOException;
+
+ /**
+ * Clean BucketCache
+ */
Review Comment:
Nit: Let's be more specific. What does it mean to clean BucketCache? We are
evicting everything? And why this is only for BucketCache, not for all
BlockCache implementations?
--
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]
