anmolnar commented on PR #8844: URL: https://github.com/apache/hbase/pull/8844#issuecomment-6801424727
Thanks @VladRodionov . I'm not sure about what has been changed exactly, I suggest to multiple commits onto the PR rather than force pushing a single commit to get better visibility on the different changes. Let me share Claude's comments on the latest version. > **AI-assisted review** — drafted by Claude (Anthropic) on behalf of @anmolnar Thanks for the quick turnaround and the explanations. I compared `2bb2bcd` against `c6d4b7f` and can confirm the points from my previous review are addressed. On your two replies: - `CacheAccessServiceTestInstance`: agreed. Keeping a single transitional fixture makes sense given it will be removed with the legacy tests, and failing explicitly in `blockCache()` covers the concern I had. - `BlockCacheTestUtils` duplication: agreed, fine to keep the two overloads explicit until the `BlockCache` one is removed. A few small things from the update itself: **1. `TestCacheOnWrite.isBucketCacheL2` is unused** The new private helper has no callers, and the `Optional` import exists only for it. The `cacheOnCompactAndNonBucketCache` condition checks only `CacheTier.SINGLE`. Should the helper be part of that condition, or can it be removed? **2. `CacheTier.SINGLE` lookup in the same condition** `CacheAccessServiceTestFactory.getCacheEngine(cache, CacheTier.SINGLE)` throws if the tier is absent. Can you confirm none of the parameterised caches in this test is a tiered (L1/L2) topology, or handle that case? **3. `@RestrictedApi` import in `LruCacheEngine`** The diff adds the annotation without an import hunk. Fine if the file already imports it; just flagging in case. **Nits** - `CacheAccessServiceTestInstance.blockCache()`: the Javadoc still says it returns `null` for a native engine, but it now throws `IllegalStateException`. The `blockCache` field is also no longer read after this change. - `TestScannerSelectionUsingKeyRange`: `cache.shutdown()` is called before `closeRegionAndWAL(region)`; closing the region first would be safer. Once CI is green on this commit (and with a note on which test was flaky earlier), I'm happy to approve. -- 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]
