dmsolr commented on PR #7880: URL: https://github.com/apache/shenyu/pull/7880#issuecomment-6362769442
Thanks @Aias00 for the thorough review. Addressed as follows. ### π΄ Merge conflict β resolved (`2e035eb95`) Merged current `master` into the branch (merge commit, **no force-push / history rewrite**). The only conflict was in `UpstreamCacheManager`, between this PR's read-only snapshot model and master's legacy gray / canary routing (`findAllUpstreamListBySelectorId`, the #7986 area). Resolution keeps both intents: - `findAllUpstreamListBySelectorId` now returns the published read-only snapshot (`task.getHealthySnapshot(selectorId)`) instead of a per-request `new ArrayList<>(...)`, so the canary path is zero-copy too, with the same `null`-when-absent contract. - `findUpstreamListBySelectorId` keeps master's gray-exclusive narrowing (`stream().filter(isGray).collect(toList())`) on top of that snapshot. This is safe because every consumer only reads: `UpstreamLabelUtils.filter` (javadoc: *"without modifying the supplied list or nodes"*) and the fixed `CircuitBreakerCandidates.filter` both return new lists. Snapshot publication in `UpstreamCheckTask` still covers all `healthyUpstream` mutators (`putToMap` / `removeFromMap` / `triggerRemoveAll`) under the lock, so the snapshot stays authoritative for the canary read. `mergeable` is now `MERGEABLE`. ### π΄ BLOCKER (immutable snapshot mutated in place) β fixed earlier in `a83c7997f` Took your **preferred option 1**: `CircuitBreakerCandidates.filter` keeps the happy path zero-copy (nothing blocked β hand back the immutable snapshot untouched, `keys[]` aligned 1:1) and only materialises a mutable `new ArrayList<>(size)` working copy on the degraded path (an upstream actually blocked, or a half-open probe granted); `probeCandidates` now uses a `List.of(upstream)` single-element view. Added the regression test you asked for β `DividePluginImmutableSnapshotTest` drives the **real** `UpstreamCacheManager` singleton snapshot (not a mock) with one of two upstreams tripped, and asserts the snapshot is immutable; `CircuitBreakerCandidatesTest`'s old `assertSame` (which pinned the in-place-compaction contract) was relaxed accordingly. ### π’ Non-blocking observations - **#2 dead branch** and **#4 `List.copyOf` rejects null** β kept defensively and now documented in the `publishHealthySnapshot` javadoc (why the `isNull` ternary is retained, and the no-null-element invariant). - **#3 null-vs-empty contract** β `getHealthySnapshot` javadoc now states the contract explicitly (`null` when the selector has no entry; otherwise an immutable list that may be empty). - **#1 O(nΒ²) publication on refresh** and **#5 staleness hazard** (`getHealthyUpstream()` exposing the live map / `submit` storing the live COW list) β deliberately **deferred**: both require changing the publication model on the health-check hot path andζΆζ the cross-module public read path, which is higher-risk than this perf change warrants. Happy to track them in a follow-up if you'd like. ### π‘ CI CI was previously not dispatched for the old fork head; it now runs on `apache/shenyu` directly and has been re-triggered on `2e035eb95`. The one earlier failure, `build (shenyu-integrated-test-http)`, was `CryptorResponsePluginTest.testWhenDoesntExistFiled` (expected `"10001"` but was `null`) β a known integration-test config-sync isolation flake, unrelated to this PR (which touches only `shenyu-loadbalancer` and `shenyu-plugin-divide`, not cryptor-response). The `UnsupportedOperationException` lines in that log are `ReadOnlyHttpHeaders.setContentLength` on an already-committed `400` response (`WebFluxResultUtils`), also unrelated to the snapshot list. Local verification of both affected modules: ``` ./mvnw -pl shenyu-loadbalancer,shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-divide -am test β BUILD SUCCESS ``` including master's `DivideCanaryRoutingTest` (11) and `DivideUpstreamDataHandlerTest` (23), plus `UpstreamSnapshotPublishTest` (5), `CircuitBreakerCandidatesTest` (9), `DividePluginImmutableSnapshotTest` (1) and `DividePluginCircuitBreakerTest` (18) β all green, checkstyle clean. -- 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]
