[chaooil]

Refactor Selector #6290

Make sure that:

• You have read the contribution guidelines.
• You submit test cases (unit or integration tests) that back your changes.
• Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

[Copilot]

Pull request overview

This PR refactors selector/rule matching in AbstractShenyuPlugin to properly leverage the existing two-level match cache (L1 WindowTinyLFU + L2 matching strategy), and adds targeted tests to cover L1 cache-hit behavior.

Changes:

• Fix selector/rule execution flow to return immediately on L1 cache hits (including cached negative-match sentinels).
• Refactor match-caching logic via a shared cacheMatchData(...) helper and minor hot-path optimizations in matching.
• Add unit tests validating L1 cache-hit and negative-sentinel short-circuit behavior for both selector and rule matching.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
shenyu-plugin/shenyu-plugin-base/src/main/java/org/apache/shenyu/plugin/base/AbstractShenyuPlugin.java	Refactors selector/rule lookup to correctly use L1 cache hits and consolidates match-caching logic.
shenyu-plugin/shenyu-plugin-base/src/test/java/org/apache/shenyu/plugin/base/AbstractShenyuPluginTest.java	Adds tests covering selector/rule L1 cache hits and negative-sentinel short-circuit cases.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Aias00]

Thanks @2823602869!

This refactor makes the L1 match cache actually work as designed. In the current code, even after an L1 hit, execute() unconditionally overwrites the result with defaultMatchSelector/defaultMatchRule, so positive L1 entries were dead weight and every request paid for full L2 matching. The new twoLevelCacheLookupSelector/twoLevelCacheLookupRule short-circuit on an L1 hit and only fall to L2 on a miss, which finally matches the documented intent of the cache (and the negative-cache/sentinel behavior is preserved via the isBlank(id) → null-handle check).

I verified the safety of the positive-hit short-circuit: CommonPluginDataSubscriber invalidates MatchDataCache on selector/rule update, delete, and refresh events (including the empty sentinels via removeEmptySelectorData/removeEmptyRuleData), so data changes still propagate to the L1 layer before a stale entry could be served.

The other parts are clean as well:

• The generic cacheMatchData + CacheWriter dedupes two near-identical blocks without behavior change.
• The loop-based matchSelector/matchRule with LinkedHashSet preserves the old distinct() + ordering semantics while cutting stream allocations on the hot path; manyMatchSelector/manyMatchRule produce the same grouping/max-key/min-sort result as before.
• The four new tests pin the L1-hit and sentinel short-circuit behavior for both selector and rule paths, and the full CI matrix is green.

Two minor notes (non-blocking):

1. cacheMatchData treats only Objects.isNull(id) as the sentinel case, whereas the old code used StringUtils.isBlank(id) — equivalent for the built-in sentinels (null id), but if a selector ever carried an empty-string id it would no longer be negatively cached; it would just re-match, so worst case is a performance edge, not correctness.
2. The file is missing a trailing newline now — checkstyle evidently tolerates it (CI is green), but worth fixing in a squash.

LGTM, approving.

[Aias00]

REQUEST_CHANGES (PMC Aias00): this PR has merge conflicts (CONFLICTING) and cannot be merged. Please rebase onto the latest master and resolve the conflicts, then request a re-review.

[Aias00]

pls fix conflicts

[chaooil]

@Aias00 Resolved conflicts

[Aias00]

Heads up: this helper is now redundant with what master already does inline.

When this PR was opened (2026-04-30), execute() did:

selectorData = defaultMatchSelector(exchange, selectors, path);   // unconditional
if (Objects.isNull(selectorData)) { return handleSelectorIfNull(...); }

so a positive L1 hit was thrown away and re-matched every request — that was the real bug you were fixing here.

But #7055 ("Fixed selector and rule match bug", merged 2026-09-11) already changed it to:

if (Objects.isNull(selectorData)) {
    selectorData = defaultMatchSelector(exchange, selectors, path);
    ...
}

This branch merged master on 2026-09-22, so it now carries that fix, and twoLevelCacheLookupSelector re-implements exactly the semantics execute() already has. Same for twoLevelCacheLookupRule — the only surviving delta is Objects.isNull(id) -> StringUtils.isBlank(id).

Not a defect, but please decide explicitly: either drop these two wrappers and keep only the isBlank tightening (smallest possible diff), or keep them purely as named helpers and say so in the PR description. Right now the commit message ("Make selector/rule L1 match cache effective") describes a bug that is already fixed on master, which will confuse whoever reads the git history later.

[Aias00]

The negative-sentinel predicate is not the same on the write/read path and the eviction path.

• Here (and in execute(), after this PR): StringUtils.isBlank(id) -> treat as "cached miss", short-circuit to handleSelectorIfNull / handleRuleIfNull.
• MatchDataCache#removeEmptySelectorData (line 95) and #removeEmptyRuleData (line 190): Objects.isNull(entry.getValue().getId()) -> evict.

So an entry whose id is "" (or whitespace) is written as a negative sentinel, is served as a negative sentinel, and is never removed when the plugin/selector data changes — it can only disappear via LRU eviction. execute() is what turns isBlank into a read predicate for the rule path too, so this PR widens an existing gap rather than closing it.

Practically the id comes from admin and is never blank, so the blast radius is near zero. Still, the sentinel should be defined once. Either:

// MatchDataCache
pathSelectorCache.entrySet().removeIf(entry -> StringUtils.isBlank(entry.getValue().getId()));

or narrow both sides back to Objects.isNull(...). Please pick one and keep cacheMatchData, execute() and removeEmpty*Data consistent.

[Aias00]

Two things here:

1. The comment says "prefer plain loop over stream to reduce allocations", but on the actual hot path (exactly one match) this allocates more, not less: new LinkedHashSet<>(4) builds a HashMap with an 8-slot table plus an Entry, and then matched.iterator() allocates an iterator. The old distinct() already did hash-based dedup internally, so dedup cost is unchanged — the only thing removed is the stream pipeline. And for the multi-match case new ArrayList<>(matched) is a second allocation that did not exist before. Either drop the perf claim from the comment, or back it with a JMH number.

2. matched.isEmpty() is unreachable — matched is only assigned inside the if (...) right before an add, so it can never be empty when non-null. Objects.isNull(matched) alone is enough.

Same two points apply to matchRule below.

[Aias00]

This test does not actually pin the L1 short-circuit.

You warm L1 with the same selector that L2 matching would return, so if the L1 lookup were removed entirely the test would still pass — L2 would produce an identical SelectorData and doExecute would be called with the same arguments.

I verified this: I swapped in master's AbstractShenyuPlugin.java (61 sources recompiled), kept your new tests, and ran AbstractShenyuPluginTest — Tests run: 12, Failures: 0, Errors: 0. All four new tests pass unchanged against the pre-PR implementation.

To actually assert "L1 hit is returned without going to L2", make L1 and L2 disagree, e.g.:

// warm L1 with a *different* selector than the one L2 would match
SelectorData cached = SelectorData.builder().id("cached-id").pluginName("SHENYU")
        .enabled(true).matchRestful(false).build();
MatchDataCache.getInstance().cacheSelectorData("/http/SHENYU/SHENYU", cached, 100, 100);
...
verify(testShenyuPlugin).doExecute(exchange, shenyuPluginChain, cached, ruleData);

or assert the negative direction with verify(testShenyuPlugin, never()) on the matching path. Same applies to executeRuleL1CacheHitTest. The two *EmptySentinelTest cases do have real value (they cover the null-id short-circuit) — it is the two positive-hit cases that are no-ops.

[Aias00]

APPROVE (PMC Aias00) — my previous REQUEST_CHANGES was raised only because the branch was CONFLICTING; the author has since merged master twice and it is now MERGEABLE, so that blocker is cleared.

Correctness-wise this is safe: I checked out the branch and ran AbstractShenyuPluginTest (12/12 green, 0 failures) and the build is checkstyle-clean (0 violations across the reactor). SelectorData/RuleData both implement equals + hashCode, so replacing distinct() with a LinkedHashSet preserves the old de-duplication and encounter-order semantics.

That said, I need to correct the rationale in my earlier approval, because the situation changed underneath this PR:

1. The bug this PR was opened to fix has already been fixed on master by #7055.
When the PR was opened (2026-04-30), execute() called defaultMatchSelector(...) unconditionally, so a positive L1 hit was discarded — that was the real defect. #7055 (merged 2026-09-11) changed it to if (Objects.isNull(selectorData)) { ... }, and this branch merged master on 2026-09-22. As a result twoLevelCacheLookupSelector / twoLevelCacheLookupRule now re-implement logic that execute() already has inline. The only surviving behavioural delta is Objects.isNull(ruleData.getId()) -> StringUtils.isBlank(ruleData.getId()) (plus applying the blank-id check to L2 results as well), which is a tightening I agree with. Please either trim the two wrappers or state in the description that they are now naming-only helpers — otherwise the commit message describes a fix that git history will show as already landed.

2. The four new tests do not discriminate the new code from the old.
They warm L1 with the same selector/rule that L2 matching would return, so they pass even if the L1 lookup is removed. I verified this: swapping in master's AbstractShenyuPlugin.java (61 sources recompiled) and running the whole AbstractShenyuPluginTest gives Tests run: 12, Failures: 0, Errors: 0 — all four new tests pass against the pre-PR implementation. The two *EmptySentinelTest cases do add real value (null-id short-circuit); the two positive-hit cases need L1 and L2 to disagree to be meaningful. Details in the inline comment.

3. Non-blocking follow-ups

• Sentinel predicate is inconsistent: cacheMatchData and execute() use StringUtils.isBlank(id), but MatchDataCache#removeEmptySelectorData / #removeEmptyRuleData still evict on Objects.isNull(id) only. A blank-id entry therefore becomes a permanent negative-cache hit that data-change events never clear. Low blast radius (admin ids are never blank), but the sentinel should be defined once.
• The "hot path / reduce allocations" comments in matchSelector/matchRule are not substantiated: new LinkedHashSet<>(4) (8-slot table + iterator) is not cheaper than the old ArrayList on the single-match path, and new ArrayList<>(matched) adds an allocation in the multi-match case. Either benchmark it or drop the claim.
• Objects.isNull(matched) || matched.isEmpty() — the isEmpty() branch is unreachable.
• cacheMatchData takes 7 parameters including 4 functional getters. It does remove real duplication, but it is a matter of taste whether this is clearer than the two explicit methods it replaces; @moremind already flagged "not good refactor", so I'd like the author to reply to that point specifically before merge.

None of the above is a correctness regression, so approving. Thanks @chaooil — and sorry for the noise from my earlier review; the premise there was true in April but is no longer true after #7055.

[Aias00]

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.

[Aias00]

Re-reviewed. Two things convinced me:

1. The generic cacheMatchData helper preserves every guard from the two copies it replaces — cache disabled, null data, and matchRestful are all still excluded, and the "cache only when all conditions are URI-type" rule is unchanged.

2. More importantly, this fixes a real bug in the process. MatchDataCache.removeEmptySelectorData / removeEmptyRuleData only evicted entries whose id was null, but the sentinel written for a miss is a blank id (StringUtils.isBlank(idGetter.apply(data)) in the new helper). So a path recorded as "no match" was never evicted and kept being served as a miss even after the selector/rule data changed. Aligning the eviction predicate to StringUtils.isBlank makes write and eviction consistent.

The L1/L2 lookup consolidation reads as equivalent to the previous flow (including the negative-cache short-circuit), and the comments explaining the sentinel contract are helpful.

Two asks, non-blocking: the PR description is still the unfilled template — please write up what this changes and why; and if there is not already one, a test that a blank-id sentinel is evicted when the underlying selector/rule data changes would lock in the fix.

[Aias00]

Hi @chaooil, this PR currently has merge conflicts with the master base branch, so CI cannot validate it against the latest code. Please rebase/merge the latest master and resolve the conflicts, then push the updated branch. Once the conflicts are cleared, CI will re-run automatically and we can continue the review. Thanks!

[Aias00]

Ping on state: approved on the code, but this branch conflicts with master.

Please rebase onto master and push. The selector refactoring has moved since this was opened, so after the rebase please re-run shenyu-admin tests to confirm the refactor still composes with the current selector code.

[Aias00]

Friendly follow-up on the merge conflict — this PR has been in mergeable_state: dirty since my note on 2026-10-23 and the branch has not moved since.

master has advanced considerably in that window, and the conflict is in files this PR rewrites, so the longer it sits the more expensive the rebase gets. Nothing about the change itself is in question; it is purely a mergeability issue.

Could you rebase onto current master and push? If the conflict resolution turns out to be non-trivial, say so here and we can look at it together — a stale branch should not be what blocks a change we have already reviewed.