Repository navigation
Conversation
…tream set RoundRobinLoadBalancer keyed its weight state by the URL of the first upstream in the list, so two selectors whose lists share that address drove the same WeightedRoundRobin counters: picks in one selector perturbed the other's smooth round-robin sequence, differing weights made the mismatch check reset the shared counter on every call, the size-mismatch check fired against the polluted map and triggered full map copies, and reordering a list orphaned the old key forever. State is now keyed by the full ordered upstream identity list together with the configured weights (the HashLoadBalancer ringCache pattern), wrapped in a bounded WindowTinyLFUMap so keys orphaned by reorders or weight changes are evicted instead of leaking. Because a key now identifies its exact member set, the inner map can never hold stale entries and the manual updateLock copy/removeIf cleanup, the recycle period and the lastUpdate bookkeeping are removed. The configured weight (not the warm-up adjusted one) keys the state so warm-up ramping does not rotate pools.
markerKey serialized the identity with Gson on every call, and the round-robin state key now builds it per request and per upstream on the hot path. Precompute it in the constructor like hashKey and leastActiveKey already are; the dubbo gray-load-balance caller benefits from the same.
wy471x
left a comment
wy471x
left a comment
There was a problem hiding this comment.
Read the balancer, UpstreamIdentity, WindowTinyLFUMap, HashLoadBalancer and the new tests.
What the change gets right. Keying on the full ordered identity list plus the configured weights does fix consequences 1-3 of #8441: two selectors sharing a first address no longer share counters, the interleaved setWeight reset disappears for that shape, the size-mismatch copy path can be deleted (map membership is now implied by the key), and using the configured weight keeps warm-up ramping from rotating pools. Removing the recycle machinery, lastUpdate and updateLock is a real simplification rather than a line-count trick.
Blocking: computeIfAbsent on WindowTinyLFUMap is not atomic.
ConcurrentMap<UpstreamIdentity, WeightedRoundRobin> map =
methodWeightMap.computeIfAbsent(List.copyOf(stateKey), k -> new ConcurrentHashMap<>(16));WindowTinyLFUMap (shenyu-common/.../common/cache/WindowTinyLFUMap.java:37) extends AbstractMap and does not override computeIfAbsent, so this resolves to Map's default get-then-put. On master the same call went through MapUtils.computeIfAbsent on a ConcurrentHashMap, i.e. it was atomic.
The consequence is a new, silent counter loss: when several threads touch a key for the first time, each builds its own inner pool; one wins the put, and the others keep mutating the orphaned pool. Their increaseCurrent() / sel() increments vanish, so the surviving pool's sequence is skewed and its neighbours get extra traffic until the key is next evicted. The new sharedUpstreamSelectorsStayIsolatedUnderConcurrency test cannot catch this, because its two threads use two different keys.
Suggested fix: add computeIfAbsent to WindowTinyLFUMap delegating to cache.asMap().computeIfAbsent(key, mappingFunction) (Caffeine's ConcurrentMap view is atomic), and add a test where several threads race on the same new key.
Also worth addressing
- Eviction resets distribution. Pools are bounded at
Constants.CACHE_MAX_COUNT(1000), and keys multiply per selector, per configured weight revision, and per circuit-breaker / canary subset (DividePluginpassesbreakeredUpstreamList). A freshly created pool starts atcurrent = 0, where the heaviest upstream wins every call until the cycle settles — so an evicted live pool degrades to 100% on one endpoint for a while. For a small upstream set that is worse than the bug being fixed. Please state the intent: is 1000 comfortably above the realistic key count, or should the seed be derived from the previous pool when one is being replaced? - Warm-up wording. The comment claims the configured weight means warm-up "does not rotate pools and reset the counters each call". The pool-rotation half is true, but inside the pool
weight != weightedRoundRobin.getWeight()still fires on every warm-up tick, becausegetWeight(upstream)is warm-up adjusted andsetWeightzeroescurrent. That is pre-existing, but the comment currently reads as if counters are preserved. reorderedUpstreamListStartsFreshStatecannot fail on the old implementation. The old key was the first URL, so reordering[a, b]→[b, a]already changed the key and already started fresh. As written the test documents behaviour instead of guarding it; consider asserting something the old code would actually break.UpstreamIdentitynow computesmarkerKeyeagerly, and whenhasVariantis true it serialises the same array twice —hashKeyandmarkerKeyare identical in that branch.this.markerKey = ...; this.hashKey = hasVariant ? markerKey : url;saves one Gson round-trip per identity.stateKeyis rebuilt on everydoSelectwithStringconcatenation plusList.copyOf. The memoisedmarkerKeyalready helps, but on the gateway hot path this is one allocation per upstream per request; caching the key per selector (the wayHashLoadBalancercachesringKey) would remove it entirely.methodWeightMapno longer holdsString -> map;poolByUpstreamSetwould describe its contents honestly.
Verdict: request changes. The direction is right and covers most of the issue, but the non-atomic pool creation silently discards round-robin state under concurrent load. Please fix WindowTinyLFUMap.computeIfAbsent (or use Caffeine's atomic get(key, mappingFunction)), add a same-key concurrency test, and clarify the eviction and warm-up comments.
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
PR #8476 fixes issue #8441 by isolating smooth weighted round-robin state per upstream set. The RoundRobinLoadBalancer was keying its state by only the first upstream's URL, causing selectors sharing that address to corrupt each other's counters. The fix keys state by the full ordered list of (markerKey + configured weight) for each upstream, uses a bounded WindowTinyLFUMap cache, and removes the lock/recycle logic. Five new tests verify isolation across shared upstreams, weight changes, reordering, and concurrency.
Evidence
- Complete diff read: 3 files modified (UpstreamIdentity.java, RoundRobinLoadBalancer.java, RoundRobinLoadBalanceTest.java) across 15585 chars, all pages consumed (has_more=false).
- All CI checks passed: CodeQL, build, pr_build, registry_dependencies (38 total checks).
- Five new tests cover the exact #8441 scenarios: shared first upstream with different weights, concurrent access, same upstream set with swapped weights, weight change rotation, and reordering fresh state.
Recommended next steps
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 3a3cc679-fd5b-43b9-b6dd-32b32f68dc55 · Commit: 6d5228e38b72d8e9344fc78a661d3ab5c744d890
Review verdict: PASS
Complete diff coverage with passing CI. The fix correctly addresses the root cause (state keyed by first URL only) by using full upstream identity + configured weight as key, with bounded cache eviction. No concrete blocking defects found.
Publication: APPROVE (EXPLICIT_PASS)
Fixes #8441.
RoundRobinLoadBalancerkeyed its smooth weighted round-robin state by theURL of the first upstream in the list. Two selectors whose lists share
that address therefore drove the same
WeightedRoundRobincounters: picksin one selector perturbed the other's sequence, differing weights for the
shared upstream made the weight-mismatch check reset the shared counter on
every call, the size-mismatch check fired against the polluted map and
triggered full map copies under the update lock on every call, and
reordering a selector's upstream list orphaned the old key forever (outer
entries were never evicted).
What changed
the configured weights — the same shape
HashLoadBalanceralready usesfor
ringCache. Two selectors sharing one address get separate pools, andselectors over the same address set with different weights stay isolated
too, because the weights are part of the key.
WindowTinyLFUMap(Constants.CACHE_MAX_COUNT), sokeys orphaned by reorders or weight changes are evicted instead of
leaking. Weight changes already reset counters by design (
setWeightclears
current), so rotating to a fresh pool preserves that semantic.never hold stale entries: the manual
updateLockcopy/removeIf cleanup,the 60 s recycle period and the
lastUpdatebookkeeping are removedoutright.
so warm-up ramping does not rotate pools and reset counters on every call
during ramp-up. The warm-up adjusted weight still drives selection and
the in-pool weight-mismatch refresh exactly as before.
Behavior note: after a configured weight change or an upstream reorder the
pool restarts from zero — the same reset the old implementation already
applied on weight changes. Orthogonal to #8327 (weight-accumulation
overflow); no wire-format or configuration change.
Tests
Five new deterministic cases in
RoundRobinLoadBalanceTest(10 000 picks,smooth-cycle exact counts):
A1,B9vsA9,C1)stay exactly 1000/9000 and 9000/1000;
A1,B9vsA9,B1) staysisolated — this case is red on the old keying (the light selector gets
0 picks out of 10 000, weight flips reset the shared counter every call);
ratio;
Module results on the current master baseline:
shenyu-loadbalancer129,shenyu-plugin-divide167,shenyu-plugin-base418,shenyu-plugin-httpclient183 — all green, zero changes outside the balancer; checkstyle clean.