Aias00 commented on PR #6336: URL: https://github.com/apache/shenyu/pull/6336#issuecomment-5776341116
I pushed the review follow-ups directly to this branch (commit 1860ebe) rather than leaving them as comments only. Three files changed, all verified locally with `mvn -pl shenyu-plugin/shenyu-plugin-base -am test` → `BUILD SUCCESS`, 18/18 tests, 0 checkstyle violations. **1. `MatchDataCache` — sentinel eviction predicate aligned (the only real issue I found)** `removeEmptySelectorData` (L95) and `removeEmptyRuleData` (L190) evicted on `Objects.isNull(id)`, while `AbstractShenyuPlugin` *writes* the negative sentinel and *short-circuits* on it with `StringUtils.isBlank(id)`. A blank-id entry was therefore served as a cached miss forever and never cleaned when the data changed — it could only disappear via LRU. Both now use `StringUtils.isBlank(...)`, so write / lookup / eviction agree. Eviction getting broader can only drop a cache entry (causing a re-match), never produce a wrong routing decision, so this is safe. Blast radius is theoretical today (admin ids are never blank) but the gap was real — and note it already existed on `master` for the selector path, since `cacheSelectorData` has always used `isBlank`. **2. `AbstractShenyuPluginTest` — the two L1 cache-hit tests now actually test something** Before: they warmed L1 with the *same object* L2 matching returns, so they passed even with the L1 lookup removed. I verified this — swapping in `master`'s `AbstractShenyuPlugin.java` and running the class gave 12/12 green. After: L1 is warmed with a deliberately distinct instance (same `id` so the rule lookup still resolves, different `name` so `equals()` distinguishes them), and `doExecute` is asserted against that instance. Verification: I temporarily made `obtainSelectorDataCacheIfEnabled` / `obtainRuleDataCacheIfEnabled` return `null` and reran — now ``` [ERROR] executeSelectorL1CacheHitTest <<< FAILURE! [ERROR] executeRuleL1CacheHitTest <<< FAILURE! ``` which is exactly what these tests should have been doing all along. The two `*EmptySentinelTest` cases were left alone; they already had value. **3. `AbstractShenyuPlugin` — dead branch + inaccurate comment** - `Objects.isNull(matched) || matched.isEmpty()` → `Objects.isNull(matched)`. The `isEmpty()` half is unreachable: the set is only assigned immediately before an `add`. - Removed the "hot path: prefer plain loop over stream to reduce allocations" comments. They are not accurate — `new LinkedHashSet<>(4)` builds an 8-slot hash table plus an iterator, which is not cheaper than the previous `ArrayList` on the single-match path, and the old `distinct()` was already hash-based so dedup cost is unchanged. The loop itself is fine and stays; only the claim goes. **Deliberately not changed:** `twoLevelCacheLookupSelector` / `twoLevelCacheLookupRule`. Per #7055 they duplicate logic `master` now has inline, but they are harmless and arguably more readable, and @moremind already has an open opinion on this refactor — I'd rather the two of you settle that than have me unilaterally delete your structure. I documented the #7055 overlap in the PR description so the history reads correctly. @moremind — your two `CHANGES_REQUESTED` are still the blocker here; my approve does not clear them. The `volatile` one is already gone from the code; the "not good refactor" one still needs an answer from the author. -- 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]
