anmolnar commented on PR #8801: URL: https://github.com/apache/hbase/pull/8801#issuecomment-6613727177
_Third-round review by Claude (AI), on behalf of @anmolnar Based on 39faf70 vs the previous 5eaad37; not built or run locally._ Thanks, this looks good to me. Final newlines are restored, `blockCacheView.jsp` and `HRegion#isPersistentBucketCacheL2` now go through `getEngine(...)`, the LRU-only test covers the original regression case with a real `CacheAccessService`, and defaulting `HRegion.cacheAccessService` to `NoOpCacheAccessService` closes the last null gap. Correction to my previous round: the `[]`-vs-HTML mismatch I described in `blockCacheView.jsp` can only happen with a `BlockCacheBackedCacheAccessService`, which an RS never builds, so it wasn't a real bug. Optional cleanups, here or in a follow-up: - The remaining `instanceof TopologyBackedCacheAccessService` guards in `isPersistentBucketCacheL2` and `blockCacheView.jsp` are now redundant, since `getEngine` works for every implementation. - `blockCacheView.jsp`: tab indentation and a trailing tab on the new lines; `CacheTopology`/`CacheTopologyType` imports are unused (as are the topology imports in `blockCache.jsp`). - `TestHRegion`: the mock tests still stub `getTopology()`/`getType()`, which are no longer called; the new LRU test could `shutdown()` the service it creates. - PR description: worth noting that `blockCacheView.jsp` now responds with `application/json` and `[]` when a tier is missing. - For the eventual master merge: release note for the `HBaseTestingUtility` `BlockCache` → `CacheAccessService` signature change. +1 from me. -- 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]
