Copilot commented on code in PR #8801:
URL: https://github.com/apache/hbase/pull/8801#discussion_r4403166505
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -1716,20 +1715,9 @@ public Map<byte[], List<HStoreFile>> close(boolean
abort, boolean ignoreStatus,
try {
synchronized (closeLock) {
if (isGracefulStop && rsServices != null) {
- rsServices.getBlockCache().ifPresent(blockCache -> {
- if (blockCache instanceof CombinedBlockCache) {
- BlockCache l2 = ((CombinedBlockCache)
blockCache).getSecondLevelCache();
- if (l2 instanceof BucketCache) {
- if (((BucketCache) l2).isCachePersistenceEnabled()) {
- LOG.info(
- "Closing region {} during a graceful stop, and cache
persistence is on, "
- + "so setting evict on close to false. ",
- this.getRegionInfo().getRegionNameAsString());
- this.getStores().forEach(s ->
s.getCacheConfig().setEvictOnClose(false));
- }
- }
- }
- });
+ LOG.info("Closing region {} during a graceful stop, so setting evict
on close to false.",
+ this.getRegionInfo().getRegionNameAsString());
+ this.getStores().forEach(s ->
s.getCacheConfig().setEvictOnClose(false));
}
return doClose(abort, status);
}
Review Comment:
This changes behavior from the previous guarded logic (only disabling
`evictOnClose` when L2 was a persistent `BucketCache`) to disabling eviction
for all graceful-stop region closes. That can retain blocks for regions that
are moved away, wasting cache space and impacting other regions while the RS is
still running. Consider restoring the conditional behavior by checking whether
the underlying cache/tier supports persistence (via CAS/topology/engine
metadata) before forcing `evictOnClose(false)`.
##########
hbase-server/src/main/resources/hbase-webapps/regionserver/blockCacheStats.jsp:
##########
@@ -18,12 +18,13 @@
*/
--%>
<%@ page contentType="text/html;charset=UTF-8"
- import="org.apache.hadoop.hbase.io.hfile.BlockCache"
+ import="org.apache.hadoop.hbase.io.hfile.cache.CacheAccessService"
+
import="org.apache.hadoop.hbase.io.hfile.cache.NoOpCacheAccessService"
import="org.apache.hadoop.util.StringUtils.TraditionalBinaryPrefix" %>
<%
- BlockCache bc = (BlockCache) request.getAttribute("bc");
-if (bc == null) { %>
+ CacheAccessService bc = (CacheAccessService) request.getAttribute("bc");
+if (bc instanceof NoOpCacheAccessService) { %>
<p>BlockCache is null</p>
Review Comment:
The message 'BlockCache is null' is no longer accurate when the cache is
represented by `NoOpCacheAccessService`. Update the UI text to reflect that
caching is disabled/unconfigured rather than null, to avoid confusion during
troubleshooting.
##########
hbase-server/src/main/resources/hbase-webapps/regionserver/blockCacheView.jsp:
##########
@@ -19,44 +19,68 @@
--%>
<%@ page contentType="text/html;charset=UTF-8"
import="java.util.*"
- import="org.apache.hadoop.hbase.io.hfile.BlockCache"
import="org.apache.hadoop.conf.Configuration"
import="org.apache.hadoop.hbase.io.hfile.BlockCacheUtil"
import="org.apache.hadoop.hbase.io.hfile.CachedBlock"
+ import="org.apache.hadoop.hbase.io.hfile.cache.CacheAccessService"
+ import="org.apache.hadoop.hbase.io.hfile.cache.CacheEngine"
+ import="org.apache.hadoop.hbase.io.hfile.cache.CacheTier"
+ import="org.apache.hadoop.hbase.io.hfile.cache.CacheTopology"
+ import="org.apache.hadoop.hbase.io.hfile.cache.CacheTopologyType"
+
import="org.apache.hadoop.hbase.io.hfile.cache.TopologyBackedCacheAccessService"
import="org.apache.hadoop.hbase.regionserver.http.RSStatusConstants"
%><%
- // This template is used to give views on an individual block cache as JSON.
+ // This template is used to give views on an individual cache tier as JSON.
Configuration conf = (Configuration) request.getAttribute("conf");
String bcn = (String)
request.getAttribute(RSStatusConstants.BLOCK_CACHE_NAME);
String bcv = (String) request.getAttribute(RSStatusConstants.BLOCK_CACHE_V);
- BlockCache bc = (BlockCache) request.getAttribute("blockCache");
+ CacheAccessService cacheAccessService =
+ (CacheAccessService) request.getAttribute("blockCache");
- BlockCache [] bcs = bc == null ? null : bc.getBlockCaches();
+ if (!(cacheAccessService instanceof TopologyBackedCacheAccessService)) {
+ System.out.println("There is no topology-backed block cache");
+ return;
+ }
Review Comment:
This JSP is used as a JSON endpoint; returning early without writing a JSON
payload produces an invalid/empty response that can break clients. Also,
`System.out.println` is not appropriate for server-side logging. Consider
emitting a valid JSON response (e.g., an empty array/object) and/or setting an
HTTP error status, and use the standard logging facility rather than stdout.
--
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]
