anmolnar commented on PR #8801:
URL: https://github.com/apache/hbase/pull/8801#issuecomment-6507643700
Initial review feedback from Claude. I haven't verified its claims yet.
---------
_Review by Claude (AI), on behalf of @<your-handle>. Based on reading
44b6842 against its parent; I didn't build or run the tests._
Thanks for pushing this forward. Moving RS ownership to `CacheAccessService`
is the right next step for HBASE-30018, and most of the caller migration
(`RSRpcServices`, `MetricsRegionWrapperImpl`, `HStore`,
`ClientSideRegionScanner`) is a clean one-for-one swap. Registering the service
once from `HRegionServer` instead of as a child of every `CacheConfig` is an
improvement too: before, closing a store could deregister the RS-wide service.
**Blocking**
1. **Graceful stop now always disables evict-on-close (`HRegion.close`).**
Previously `evictOnClose` was turned off only when L2 was a `BucketCache` with
persistence enabled. Now it's turned off for every topology. On a plain LRU
setup, or a bucket cache without persistence, a closed region's blocks now stay
cached on a draining server, which contradicts "no user-visible behavior
change". I'd keep the check by adding a capability to the new API, e.g.
`isPersistenceEnabled()` on `CacheEngine`/`CacheAccessService` (default
`false`), with `BlockCacheBackedCacheEngine` delegating to
`BucketCache.isCachePersistenceEnabled()`, or by checking
`PersistentCacheComponent` on the L2 engine. A test for both cases (persistence
on and off) would lock this in. (Same point Copilot raised.)
2. **Removed tests with no replacement.**
`TestCacheConfig#testRegistersCacheAccessServiceAsConfigurationChild` and
`#testDeregistersCacheAccessServiceAsConfigurationChild` are gone. Could we add
a test showing the RS registers the service with `ConfigurationManager`, a
config reload reaches the engines, and closing a region doesn't drop that
registration?
**Non-blocking, but worth doing**
3. **`blockCacheLevel.jsp`:** `isLru` now checks `instanceof
LruCacheEngine`. `IndexOnlyLRU` is still a legacy `IndexOnlyLruBlockCache`
(extends `LruBlockCache`) wrapped in `BlockCacheBackedCacheEngine`, so its
index/bloom count and size rows disappear. Unwrapping as the JSP already does
for `BucketCache` would fix it.
4. **`LruCacheEngine#getCachedFileNamesForTest` / `#getMapForTests` made
`public`.** The latter exposes the live map. Could these use
`@RestrictedApi(allowedOnPath = ".*/src/test/.*")` instead?
5. **Concrete-class branching.** `HeapMemoryManager#toResizableCacheEngine`
throws `IllegalArgumentException` for any service that isn't topology- or
`BlockCache`-backed. `HeapMemoryManager`,
`MetricsRegionServerWrapperImpl#initBlockCache` and `blockCache.jsp` each
repeat the "SINGLE vs L1 tier" selection. A helper on the service (e.g.
`Optional<ResizableCacheEngine> getResizableHeapEngine()` or
`getEngine(CacheTier)`) would remove both problems.
6. **Unreachable branches.** `CacheAccessServices.fromConfiguration` only
returns a topology-backed or no-op service, so the
`BlockCacheBackedCacheAccessService` branches in `HeapMemoryManager` and
`MetricsRegionServerWrapperImpl` can't run in an RS, while the JSPs don't
handle that case at all. I'd either drop the branches or make the JSPs handle
the same case.
7. **`HRegion.cacheAccessService` can still be null.**
`setCacheAccessService(null)` turns null into `NoOpCacheAccessService`, but the
field stays null when `rsServices == null` and the setter is never called.
Initializing it to `NoOpCacheAccessService` would make this consistent with
`CacheConfig`, which already treats null as disabled.
**Nits**
- `blockCacheView.jsp`: writes "no tier" messages to `System.out` and
returns an empty body from a JSON endpoint. Not new behavior, but returning
`[]` would be nicer while the file is being rewritten.
- `blockCacheStats.jsp`: "BlockCache is null" → "Block cache is disabled".
- `BlockCacheUtil#toJSON(CacheAccessService)` has no callers, and with
`GSON` excluding private fields it would print `{}`. Suggest removing it.
- JSPs mix tabs and spaces, have trailing whitespace on import lines, and
two are missing a final newline.
- `HRegionServer#getCacheAccessService` javadoc still says "block cache
instance" and "which not carry table".
- `HBaseTestingUtility` is `@InterfaceAudience.Public` (deprecated), so the
`createRegionAndWAL(..., BlockCache)` and `createTestRegion(..., BlockCache)`
signature changes will need a release note for the eventual master merge, or
keep `BlockCache` overloads that adapt through `CacheAccessServices`.
Happy to take another look once #1 and #2 are addressed.
--
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]
