perf(plugin-base): batch namespace snapshot cache updates and invalidation (#7645)

[dmsolr]

Which issue is this PR closes?

Closes #7645.
Tracker: #7650.

What does this PR change?

CommonPluginDataSubscriber.applySelectorDataSnapshot / applyRuleDataSnapshot previously routed every incoming and every stale row through the per-row subscribe path. For each row that meant:

• one ConcurrentMap.compute on selectorMap / ruleMap that rebuilt a full ArrayList, ran List.sort(...) and produced a fresh List.copyOf(...) — one merge+sort per row instead of one per plugin/selector bucket;
• one match-cache invalidation cycle (removeSelectorData(plugin, id) + removeEmptySelectorData(plugin) and the rule-side equivalents) per row.

With a cold-init or reconnect snapshot of a few thousand rows this dominated sync-apply CPU and allocation, even though BaseDataCache already exposed grouped refreshSelectorData / refreshRuleData helpers used by onSelectorRefresh / onRuleRefresh.

This PR reuses that machinery on the snapshot path and adds a symmetric batch removal helper.

BaseDataCache

Two new package-private helpers:

• removeSelectDataBatch(List<SelectorData>) groups the batch by pluginName and does one computeIfPresent per bucket, filtering by an id HashSet. Dangling selectors (blank plugin name) still match every bucket, mirroring removeSelectData(SelectorData).
• removeRuleDataBatch(List<RuleData>) groups by selectorId with the same one-merge-per-bucket shape.

CommonPluginDataSubscriber

applySelectorDataSnapshot(namespaceId, dataList):

1. Compute the stale rows exactly as before (namespace filter + incoming-id exclusion).
2. One refreshSelectorData(dataList) — grouped by plugin, merge+sort once per plugin bucket.
3. One removeSelectDataBatch(stale) — grouped by plugin, single merge per bucket.
4. Iterate dataList and stale to fire per-row plugin-handler callbacks (handlerSelector / removeSelector). Callbacks are wrapped in a new runGuarded helper that preserves the exception semantics of subscribeDataHandler: rethrow as IllegalStateException during InitialSyncApplication, otherwise log and continue so the rest of the snapshot still converges.
5. Coalesce match-cache invalidation per affected pluginName via invalidateSelectorMatchCacheByPlugins — uses the broader removeSelectorData(pluginName) / removeRuleData(pluginName) variants, so both positive and negative entries under the touched plugins are cleared together. Other plugins caches are untouched.

applyRuleDataSnapshot mirrors that shape with refreshRuleData / removeRuleDataBatch / invalidateRuleMatchCacheByPlugins.

Plugin-handler contracts, deletion semantics, ordering by sort, namespace isolation and empty-snapshot cleanup are unchanged. applyPluginDataSnapshot is untouched: pluginMap.put is already O(1) and there is no per-row sort to coalesce.

Ordering vs. concurrent incremental updates

The batch cache write is a single reference swap (selectorMap = next / ruleMap = next) built from the current map — the same shape as onSelectorRefresh. Per-row handler callbacks run after the swap so they observe the fully merged snapshot. Concurrent onSelectorSubscribe calls arriving mid-snapshot can still race with the swap; that race exists on onSelectorRefresh today and is not widened by this change.

Tests

New unit tests in CommonPluginDataSubscriberTest:

• selectorSnapshotBatchesMergesAndPreservesSortOrder / ruleSnapshotBatchesMergesAndPreservesSortOrder — incoming rows out of order plus a replacement for an existing id must produce a single sorted list per bucket, and every row must still hit handlerSelector / handlerRule.
• selectorSnapshotInvokesRemoveSelectorForEachStaleRow / ruleSnapshotInvokesRemoveRuleForEachStaleRow — empty incoming snapshot clears only the stale rows in the target namespace, keeps other namespaces, and calls removeSelector / removeRule per stale row.
• snapshotCoalescesMatchCacheInvalidationPerAffectedPlugin — positive and negative match-cache entries under the affected plugin are cleared; entries under an unaffected plugin survive.
• selectorSnapshotHandlerFailurePropagatesDuringInitialSync — a throwing handler during InitialSyncApplication.run(...) aborts the snapshot as before.
• selectorSnapshotHandlerFailureIsLoggedOutsideInitialSync — outside initial sync the failure is logged and the remaining rows still converge.

Existing testSelectorSnapshotReplacesRowsAndRemovesOnlyStaleNamespaceRows and testRuleSnapshotReplacesRowsAndRemovesOnlyStaleNamespaceRows continue to pass unchanged, so the namespace-isolation contract is preserved.

Module run:

./mvnw test -pl shenyu-plugin/shenyu-plugin-base -Dtest=CommonPluginDataSubscriberTest,BaseDataCacheTest
Tests run: 60, Failures: 0, Errors: 0, Skipped: 0
./mvnw checkstyle:check -pl shenyu-plugin/shenyu-plugin-base
You have 0 Checkstyle violations.

Measurement status

The tracker issue asks for a before/after fixed-load comparison (1k / 10k+ configs, cold init and reconnect, business traffic running). This environment does not have a load generator, controlled upstream or JFR profiling harness, so no numeric before/after is attached here. The change is source-grounded: it removes an O(n) merge+sort per row and an O(n) match-cache invalidation per row on a path that already had a batched alternative, and it does not touch request-path code. Reviewers with a benchmark rig should be able to reproduce the sync-apply delta directly.

Compatibility

No public API change. removeSelectDataBatch / removeRuleDataBatch are package-private. Behavioural differences are limited to:

• Ordering of cache-write vs. plugin-handler callbacks within a single snapshot apply — callbacks now run after the full batch is merged (matches onSelectorRefresh), instead of interleaved per row.
• Match-cache invalidation is broader per touched plugin (all entries under that plugin are cleared, not just the entries whose selector/rule id changed). Broader invalidation cannot retain stale positive or negative results; the trade-off is a slightly higher refill cost right after a snapshot, bounded by the number of affected plugins.

Neither changes routing decisions.

[Copilot]

🟡 Changes recommended

Concurrent updates can be lost, failed snapshots can skip cleanup permanently, and dangling-rule matches can remain cached.

3 open findings

• Snapshot map replacement can overwrite concurrent incremental updates · New
• Snapshot failures make stale-row cleanup and cache invalidation non-retryable · New
• Blank plugin names skip cross-plugin invalidation for stale rules · New

What changed in this PR

Optimizes ShenYu’s namespace snapshot processing to reduce repeated cache copying, sorting, and invalidation during synchronization.

Changes:

• Batches selector and rule updates and stale-row removals.
• Coalesces match-cache invalidation by affected plugin.
• Adds tests for sorting, namespace isolation, callbacks, and handler failures.

File	Description
shenyu-plugin/​shenyu-plugin-base/​src/​test/​java/​org/​apache/​shenyu/​plugin/​base/​cache/​CommonPluginDataSubscriberTest.java	Adds snapshot regression tests.
shenyu-plugin/​shenyu-plugin-base/​src/​main/​java/​org/​apache/​shenyu/​plugin/​base/​cache/​CommonPluginDataSubscriber.java	Batches snapshot application and invalidation.
shenyu-plugin/​shenyu-plugin-base/​src/​main/​java/​org/​apache/​shenyu/​plugin/​base/​cache/​BaseDataCache.java	Adds grouped stale-row removal helpers.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Aias00]

Review — batch namespace snapshot cache updates and invalidation

Strong PR. The performance argument is sound, the batching shape is right, and I verified the core claims against the actual dependency sources rather than taking them on trust. I found two blocking issues, both narrow but both reachable, plus a few notes.

I diffed the whole head tree against master, so everything below is checked against the real code, not the PR description.

---

🔴 Blocking 1 — dangling (blank pluginName) selectors no longer get their match cache invalidated

CommonPluginDataSubscriber.collectAffectedPlugins (lines 456-472) filters out blank names:

if (StringUtils.isNotBlank(name)) {
    plugins.add(name);
}

That filter is correct for the handler lookups (handlerMap is a ConcurrentHashMap built with Collectors.toConcurrentMap at line 73/90, so handlerMap.get(null) would NPE — good catch putting the guard there). But it also means a selector whose pluginName is blank contributes nothing to the match-cache invalidation set.

On master, that case was explicitly handled. removeCacheData → MatchDataCache.removeSelectorData(pluginName, selectorId) and removeRuleDataBySelector(pluginName, selectorId) both have a blank-name branch that sweeps every bucket (MatchDataCache.java:79-91 and :215-227). BaseDataCache keeps the same defensive handling in removeSelectData (BaseDataCache.java:140-149), and BaseDataCacheTest has a dedicated test for it (testRemoveSelectDataSweepsAllBucketsWhenPluginNameIsMissing, line 170). Your own removeSelectDataBatch preserves this for the BaseDataCache side (lines 208-210) — it's only the match-cache side that lost it.

And such entries are creatable: the only production writer is AbstractShenyuPlugin.cacheSelectorData (:156-175), which keys by selectorData.getPluginName() and calls MapUtils.computeIfAbsent(SELECTOR_DATA_MAP, pluginName, ...) — a "" bucket is reachable.

Consequence: a snapshot that deletes a dangling selector leaves its entry in MatchDataCache, so the request path can short-circuit to a match for a selector that no longer exists in BaseDataCache.

Ask: either treat blank as "all buckets" the way MatchDataCache already does, or — if blank-pluginName selectors are now considered impossible — drop the defensive branches in BaseDataCache.removeSelectData / MatchDataCache too so the intent is uniform, and say so in the PR. As it stands the module is internally inconsistent. Please add a test either way.

The same gap applies to RuleData with a blank pluginName in invalidateRuleMatchCacheByPlugins (collectAffectedPlugins(dataList, stale, RuleData::getPluginName)), where master's invalidateRuleMatchCache reached removeRuleData(blank, id)'s sweep branch.

---

🔴 Blocking 2 — the batch cache write lost the per-row exception isolation subscribeDataHandler provided

On master, applySelectorDataSnapshot was:

dataList.forEach(this::onSelectorSubscribe);   // per row, inside subscribeDataHandler's try/catch

subscribeDataHandler (lines 288-303) catches Exception, rethrows during InitialSyncApplication, and otherwise logs and moves on. So a single malformed row was isolated: the rest of the snapshot still converged, and BaseDataCache ended up correct for every good row.

In the head, cache.refreshSelectorData(dataList) (lines 173-175) and cache.refreshRuleData(dataList) (lines 203-205) are unguarded. A row with a null pluginName produces a null key from Collectors.groupingBy (a HashMap permits it), and then next.compute(null, ...) on a ConcurrentHashMap throws. That NPE now propagates out of applySelectorDataSnapshot and aborts everything after it — the stale removal, all handler callbacks, and the match-cache invalidation. BaseDataCache itself is left untouched (the selectorMap = next assignment at BaseDataCache.java:422 is after the forEach), but the whole snapshot is dropped.

Note the NPE contract itself is pre-existing and deliberately tested — BaseDataCacheTest.invalidCandidatesDoNotPublish (lines 392-401) asserts exactly this for all three refresh*Data methods. On master that path was only reachable from onSelectorRefresh. This PR makes it reachable from the snapshot apply, without the isolation.

How far the NPE travels, verified end to end:

ShenyuWebsocketClient.onMessage            (:462-479) catch (RuntimeException) -> handleSyncFailure(...)
WebsocketDataHandler.snapshot              (:103-113) no try/catch
AbstractDataHandler.handleSnapshot         (:67-73)   no try/catch
SelectorDataHandler.doSnapshot             (:49-54)   no try/catch

handleSyncFailure (ShenyuWebsocketClient.java:493-505) increments the consecutive-failure counter and calls this.close() — i.e. a websocket reconnect for full resync. Because the same bad snapshot is re-delivered on reconnect, this becomes a reconnect loop rather than a one-off. On master the same row was logged and the connection stayed up with a correct cache.

Reachability: the row only needs a non-null SelectorData with a missing pluginName field. SelectorDataHandler.doSnapshot's namespace check (!namespaceId.equals(data.getNamespaceId())) passes as long as namespaceId is present, and the payload is Gson-deserialized with no field validation (SelectorDataHandler.convert, :38-40).

Ask: wrap the batch cache writes (and collectAffectedPlugins, which is also unguarded — pluginNameOf.apply(data) NPEs on a null element, whereas master's Optional.ofNullable(classData).ifPresent(...) at :291-296 tolerated them) in the same runGuarded treatment, or filter/skip invalid rows up front with a warning. Keep the initial-sync rethrow semantics if you want fail-fast at startup.

---

✅ What I verified as correct

Batching machinery

• refreshSelectorData / refreshRuleData are pre-existing on master (used by onSelectorRefresh / onRuleRefresh), already tested for atomicity and unrelated-bucket retention (BaseDataCacheTest.selectorRefreshRetainsMissingDataAndPublishesSortedLists, :352-370), and they publish List.copyOf(...) immutable snapshots. Reusing them is the right call.
• removeSelectDataBatch (BaseDataCache.java:192-212) is a faithful group-by of removeSelectData: skips null data and null ids, sweeps every bucket for blank plugin names, one computeIfPresent per affected bucket, empty result removes the bucket. Matches the existing single-row semantics exactly.
• removeRuleDataBatch (:288-305) is keyed by selectorId (the actual ruleMap key) and is more defensive than master's removeRuleData, which would NPE on a null selectorId via computeIfPresent(null, ...); the batch version skips those rows. Fine — stale rows come from ruleMap.values() so their selectorId is a real map key anyway.

Exception semantics

• runGuarded (lines 440-454) genuinely reproduces subscribeDataHandler. I checked InitialSyncApplication.run (shenyu-common/.../InitialSyncApplication.java:44-62): it sets the ThreadLocal before action.run() and does not swallow, and isActive() is ThreadLocal-based, so selectorSnapshotHandlerFailurePropagatesDuringInitialSync's expectation is sound.
• Per-row ordering is preserved. Master's updateCacheData also wrote the cache before invoking the handler (cacheSelectData → handleSelectorData, lines 320-323), so "cache first, then handler" still holds per row; the only change is that all cache writes now precede all handlers. For handlers that read BaseDataCache this is strictly more consistent.

Match-cache coalescing

• The coarser invalidation is safe. removeSelectorData(pluginName) is SELECTOR_DATA_MAP.remove(pluginName) (MatchDataCache.java:69-71), which subsumes master's removeSelectorData(plugin, id) + removeEmptySelectorData(plugin); removeRuleData(pluginName) is RULE_DATA_MAP.remove(pluginName) (:185-187), subsuming removeRuleDataBySelector + removeEmptyRuleData. No stale entry can survive — the cost is a wider flush, which is the deliberate trade the code comment documents.

Tests

• clearCaches() in @BeforeEach (lines 430-436) clears both the BaseDataCache and MatchDataCache singletons, so the new MatchDataCache assertions can't leak between tests.
• Coverage is good: sort-order preservation, upsert-replacement, per-row handler invocation, stale removal, namespace isolation, match-cache coalescing including "unrelated plugin's cache survives", and both initial-sync and post-initial-sync failure paths.

Concurrency claim — your PR description says the swap race "exists on onSelectorRefresh today and is not widened by this change". I confirmed that: refreshSelectorData is already called from onSelectorRefresh on master (CommonPluginDataSubscriber.java:511-517 in head, identical on master). Accurate.

---

📝 Non-blocking notes

1. applyPluginDataSnapshot is now the odd one out — still per-row onSubscribe/unSubscribe while selector and rule are batched. Your justification (pluginMap.put is O(1), no per-row sort) is correct, so this is fine, but a short TODO would help the next reader.
2. The wider match-cache flush has a real cost. A snapshot touching one selector now drops that plugin's entire selector and rule match cache. On a plugin with a large cache that's a much bigger flush per snapshot than master. Acceptable given the batching goal; just worth knowing it's not free.
3. JsonUtils.toJson(data) in runGuarded's error path would log "null" for a null row. Cosmetic.

---

CI

Both red checks are unrelated to this PR:

• e2e (3s) is the fail-fast aggregation job.
• e2e-storage (e2e-opengauss-compose) failed on DividePluginTest.testDivide:105 endpoint '/anything' not exists. DividePluginCases uses httpbin.org as a live upstream (DividePluginCases.java:69-70), so this is the same external-site flake I attributed while reviewing #7677. On master, run 38836735684 failed the identical assertion on postgres while opengauss passed (e2e-storage (shenyu-e2e-case-storage, e2e-postgres-compose) | failure, opengauss | success) — the failure migrates between databases, which rules out a database-specific regression. The three master runs since then (8f3ee32e, 66d7d954, 3af7bfc6) are all green. This PR touches no e2e code.

---

Summary

Request changes for the two items above. Both are small fixes: keep the blank-pluginName sweep for match-cache invalidation, and give the batch cache write the same per-row isolation the handler callbacks already have. Everything else — the batching shape, the immutable published snapshots, the exception semantics, the test isolation — is correct, and the perf motivation is real. Happy to re-review once those land.