anmolnar commented on PR #8801: URL: https://github.com/apache/hbase/pull/8801#issuecomment-6531407376
_Second-round review by Claude (AI), on behalf of @<your-handle>. Based on 5eaad37 vs the previous 44b6842; not built or run locally._ Thanks for the quick turnaround. The blocking items are fixed: the persistent-L2 check is back in `HRegion.close` with tests, and the RS-level config registration now has a test. Adding `getEngine(CacheTier)` / `getFirstLevelEngine()` to `CacheAccessService` is a nice cleanup; it removed the duplicated tier-selection logic from `HeapMemoryManager`, the metrics wrapper and the JSPs. A few remaining items: 1. **Trailing newlines.** All six regionserver `blockCache*.jsp` files now end without a newline (all had one on the base branch). Probably the whitespace cleanup; could you restore them? 2. **`blockCacheView.jsp`** still requires `TopologyBackedCacheAccessService` and reads its topology directly, so it returns `[]` for a block-cache-backed service while the HTML tabs show data. Using `getFirstLevelEngine()` / `getEngine(CacheTier.L2)` would make it consistent with the other pages. 3. **`HRegion#isPersistentBucketCacheL2`** could use `getEngine(CacheTier.L2)` instead of casting to the topology-backed service and checking the topology type. (Typo in the TODO: "BuckeCache".) 4. **Tests:** the `TestHRegion` cases test the helper with mocks only. A case for a single-tier/LRU-only setup returning `false` would cover the original regression. In `testCacheAccessServiceRegistrationSurvivesRegionClose`, the `notifyAllObservers(conf)` calls aren't followed by any assertion; either verify `onConfigurationChange` reaches the service (e.g. via a spy) or drop the "verify delivered" comments. 5. `blockCacheView.jsp` content type changed from `text/html` to `application/json`. That's more correct, but worth a mention in the PR description given the "no user-visible change" note. Nits: `blockCacheLevel.jsp` scriptlet lost its indentation; `startHeapMemoryManager` could check `getFirstLevelEngine().isPresent()` instead of `instanceof NoOpCacheAccessService`; `HRegion.cacheAccessService` could default to `NoOpCacheAccessService`; and the `HBaseTestingUtility` signature change still needs a release note before the master merge. LGTM 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]
