Repository navigation
[ISSUE #8300] Add the opt-in adaptive scoring strategy for divide - #8365
Conversation
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
PR #8365 adds an opt-in adaptive load-balance strategy for the divide plugin. Frozen evidence claims complete_diff=true, 64 green CI checks, +779/-5 across 7 files. Direct head-version reads in this session covered AdaptiveLoadBalancer (full), AbstractLoadBalancer.select (full), and partial AdaptiveUpstreamStats/Test, but the VERSIONED_DIFF pages were archived (not surfaced) and the modified integration files (DividePlugin, LoadBalanceEnum, SPI) were not directly read.
Evidence
- AdaptiveLoadBalancer.java (head ee6e275) read in full: p2c two-candidate pairing retained; compare() gates adaptive vs cold-start p2c scale via bothAdaptive; select() reserves in-flight on the singleton short-circuit; feedback() resolves the @Join singleton so selection and feedback share state.
- AbstractLoadBalancer.select (head ee6e275, lines 40-73) confirms the size==1 short-circuit that the adaptive in-flight accounting depends on.
- AdaptiveUpstreamStats constants (MIN_SAMPLES=5, EWMA_WINDOW_MILLIS=600, FAILURE_PENALTY_MILLIS=1000, PRUNE_THRESHOLD=256) and bothAdaptive/pruneIfNeeded verified; record()/score() bodies (lines ~79-118, 157-208) were not surfaced.
Recommended next steps
- Re-run resource_diff for base 5bd9305b / head 09a290d5 across all offsets (0..40272) and read every page to certify full line-level coverage of the +779/-5 diff.
- Read the modified DividePlugin.java (+50/-4) and LoadBalanceEnum.java (+8/-1) head versions to verify the feedback-hook contract and enum wiring.
- Read AdaptiveUpstreamStats record()/score() (lines ~79-208) to validate the failure-floor EWMA smoothing math.
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 28d372d3-d0a7-4561-985f-19897a2a1028 · Commit: ee6e2758b425284a0ef6e3a232c0120c21e6bf84
Review verdict: INCONCLUSIVE
Full diff coverage cannot be certified: all VERSIONED_DIFF pages (base 5bd9305b / head 09a290d5) returned as archived ledger observations and were not surfaced in this session, and the modified integration files DividePlugin.java (+50/-4), LoadBalanceEnum.java (+8/-1), the SPI registration file, and AdaptiveUpstreamStats record()/score() were not directly read. Per review policy, incomplete coverage must be reported as INCONCLUSIVE rather than turned into approval; no concrete HIGH/CRITICAL defect was identified in the code that was read.
Publication: COMMENT (REVIEW_EVIDENCE_MISSING)
Review details
This review cannot be finalized as PASS. The complete captured diff was not surfaced in this session (all resource_diff pages returned as archived ledger observations), and several changed files — DividePlugin.java (+50/-4), LoadBalanceEnum.java (+8/-1), the SPI registration, and AdaptiveUpstreamStats record()/score() — were not directly read. Per review policy, incomplete coverage must yield INCONCLUSIVE rather than approval. The new AdaptiveLoadBalancer code that was read looks coherent (p2c pairing preserved, adaptive/cold-start scale mixing avoided, in-flight reservation matches the divide completion decrement, feedback resolves the registered @Join singleton), but full verification of the integration contract is outstanding. Recommend re-running the versioned diff across all offsets and reading the modified files before merging.
Meeting review (request_changes) — strengthenedThree-agent meeting consensus on #8365 (linked #8300), head Must fixM1 (P1) — Bind feedback & inflight to the actual HTTP attempt
Acceptance: each real HTTP attempt gets its own monotonic start, target, headers, and terminal fact; reserve/release exactly once through body complete/error/cancel; distinguish local/client vs downstream delivery vs upstream failure. Overlap tests must assert per-attempt elapsed/target/terminal independence (same-tick equal beginTimes are fine). M2 (P1) — Probe / specify-domain reserve & real identity
Unify attempt ownership for every bypass: pair reserve/release on success/error/cancel and keep the real target identity for scoring. A non-negative score clamp is defense-in-depth only — it does not replace pairing.
M3 (P1) — Same-tick EWMA drop & 4xx time base
M4 (P2) — Bounded, scoped stats lifecycle
Required: budgeted/throttled cleanup covering singleton/churn; define stale fallback; make lifecycle contracts explicit (isolation granularity / cleanup mechanism can be author-equivalent designs if validated). M5 (P2) — One snapshot for both candidates; cold fallback lag gap
M6 (P2) — Limit adaptive to wired Divide HTTP workloads
Regression tests for M1–M6 are merge acceptance criteria (mirror existing p2c skeletons where possible), not optional follow-ups. Suggestions (non-blocking)
Bottom lineKeep the SPI and algorithm direction, but block merge until attempt ownership, probe pairing, and EWMA time-base issues are fixed and covered by tests. P2 items above are still merge-blocking for correctness/lifecycle, with room for equivalent implementations. |
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
PR #8365 adds an opt-in adaptive load-balance strategy for the divide plugin. It introduces AdaptiveLoadBalancer (p2c-style two-random pick + per-upstream EWMA latency/inflight/weight scoring), AdaptiveUpstreamStats (bounded per-identity feedback state, failure floor, prune), AdaptiveOutcome (SUCCESS/CLIENT_ERROR/SERVER_ERROR/TRANSPORT_ERROR), registers it via SPI and LoadBalanceEnum (code 7), and wires the completion trigger in DividePlugin. The change is self-contained, fully covered by 263 lines of tests, and CI passed all 64 checks. No blocking defect was found.
Evidence
- Full VERSIONED_DIFF read (40,272 chars, all pages has_more=false): 7 files — LoadBalanceEnum (ADAPTIVE code 7), AdaptiveLoadBalancer, AdaptiveOutcome, AdaptiveUpstreamStats, SPI registration, AdaptiveLoadBalancerTest (263 lines), DividePlugin integration.
- DividePlugin.adaptiveFeedback correctly pairs the reservation: upstream.getInflight().decrementAndGet() runs before the CANCEL early-return, so the in-flight count is released on every completion signal (SUCCESS/ERROR/CANCEL); sticky-binding and hash-pick branches also increment for ADAPTIVE, mirroring the p2c #7974 accounting fix.
- AbstractLoadBalancer.select short-circuits size==1 before doSelect, so AdaptiveLoadBalancer.doSelect/pickTwoUpstreams only ever see size>=2 (no divide-by-zero/NPE); the singleton reservation is made in AdaptiveLoadBalancer.select for size==1, matching the always-decrement completion path.
Recommended next steps
- Consider adding an explicit test asserting adaptiveFeedback releases the in-flight reservation on the CANCEL SignalType (the decrement-before-return is already correct, but the new suite does not appear to cover the CANCEL path).
- Optionally document the new
adaptiveload-balance option for the divide plugin in the user docs.
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 6d8fa001-19e7-4650-a885-3d86178692be · Commit: ee6e2758b425284a0ef6e3a232c0120c21e6bf84
Review verdict: PASS
Complete frozen diff reviewed end-to-end; cross-file contract (enum -> SPI -> plugin constant) is consistent; in-flight accounting is correctly paired on all selection and completion paths; cold-start always falls back to the raw p2c load formula (scales never mixed); 4xx neutral, 5xx/transport floored at 1000ms; bounded prune at 256 entries; 263 lines of new tests cover scoring, cold-start, prune, forced exploration and singleton pairing; CI 64/64 success, passed=true. No blocking defect introduced by this PR.
Publication: APPROVE (EXPLICIT_PASS)
Review details
Reviewed #8365 (ee6e275). Verdict: PASS. The full frozen diff was read end-to-end (all pages delivered, has_more=false) and every integration seam was verified: the ADAPTIVE enum, SPI registration, and DividePlugin constant line up; in-flight accounting is correctly paired on all selection paths (doSelect, singleton short-circuit, sticky-binding hit, hash pick) and always released in adaptiveFeedback via decrement-before-CANCEL-return; AbstractLoadBalancer.select's size==1 guard prevents pickTwoUpstreams from ever seeing size<2; cold-start always falls back to the raw p2c load formula (scales are never mixed); 4xx is neutral while 5xx/transport is floored at 1000ms; the feedback map is bounded (prune at 256, idle 10m). 263 lines of new tests cover scoring, cold-start, prune, forced exploration and singleton pairing, and CI is green (64/64). Recommend merging.
Part of #8300.
Problem
The divide plugin ranks its candidates with per-strategy heuristics that only see part of the picture: p2c pairs the smoothed lag with the in-flight count, but its lag attribution reads a shared per-upstream timestamp, so overlapping attempts attribute each other's elapsed time, and nothing in the selection path distinguishes a fast 5xx from a slow success — a node that fails within a millisecond looks cheaper than one that answers correctly in fifty. This adds the opt-in
adaptiveload-balance strategy as the first delivery of the adaptive scoring evaluation: selection cost stays O(1), cold start and discovery churn behave exactly like today, and no throughput improvement is claimed without the versioned benchmarks, which remain a follow-up.Approach
The new
AdaptiveLoadBalancerkeeps the p2c pairing — two random candidates compared without a full-list sort — and compares per-upstream adaptive scores instead of the raw(sqrt(lag)+1) * inflightproduct:(smoothedLatency + 1) * (inflight + 1) / max(warmUpWeight, 1), smaller is better. While a candidate is still cold (fewer than five latency samples) the pair is compared with the p2c load formula for both sides, so the two scales are never mixed inside one comparison, and restarts, discovery churn and brand-new selectors behave exactly like the current p2c. Idle nodes keep the p2c forced exploration, so a degraded node is probed again and can recover.The feedback is a single hook, not a second telemetry pipeline: the divide plugin's existing per-strategy completion trigger gains an
adaptivebranch that measures each attempt with its own start time — overlapping attempts never share a timestamp, which removes the shared-stamp attribution the p2c path carries — classifies the outcome, and hands it to the selector. The classification keeps upstream failures distinct from client-caused results: a downstream cancellation says nothing about the upstream, a 4xx is neutral (no latency recorded, no penalty), and a 5xx or a transport failure enters the smoothing as a synthetic latency sample of at least one second. That floor is what keeps a fast failing node unattractive: finishing quickly can never make its score smaller than the failure cost, and a clean node recovers because subsequent successful samples dominate the smoothing.Statistics are keyed by the existing
UpstreamIdentity, so they survive equivalent discovery refreshes, live in a bounded map (amortized prune of identities without feedback for ten minutes, only once it outgrows 256 entries), and are replaced atomically per feedback. Health checks, canary partitioning, breaker admission, sticky affinity, deterministic keys and the draining/ready gating all run before this selector sees the candidates, so the adaptive comparison only ranks already eligible targets; sticky and deterministic hits reserve their in-flight attempt with the same compensation the p2c path uses. Activation is the smallest possible surface: the newadaptiveentry of the load-balance enum, selectable per divide rule like every other strategy; benchmarking, a shadow mode and the alignment with the observation contracts of #8209/#8158 stay follow-ups, and the feedback hook is deliberately the single seam those contracts will plug into.Testing
All tests are deterministic and sleep-free. They cover the cold-start fallback comparison, warm candidates preferring the lower smoothed latency, in-flight work tipping the balance between equal latencies, a millisecond 5xx being smoothed in with the failure floor and losing to a slower healthy node, 4xx observations being neutral (no latency movement, no sample count), a transport failure entering with the floor without a real measurement, successful samples pulling the smoothing back below the floor, the forced exploration of an idle node, the singleton reservation pairing, the SPI resolution, the static feedback routing to the registered selector, and the bounded prune.
shenyu-loadbalancer107 tests (95 existing + 12 new),shenyu-plugin-divide159 tests (all existing), Checkstyle clean on shenyu-common, shenyu-loadbalancer and shenyu-plugin-divide.