hanzhe-one opened a new pull request, #7986: URL: https://github.com/apache/shenyu/pull/7986
Fixes #7972 ## Problem `LeastActiveLoadBalance` kept a private `countMap` keyed by domain. The counter was incremented on every selection and never decremented, and each entry started at `Long.MIN_VALUE`. Two consequences: - **Completed requests kept influencing the choice.** The comparison was cumulative selections, not concurrent active requests, so an upstream that had served more requests stayed "busier" forever regardless of how much work it currently had. - **A newly added upstream received a catch-up burst.** A fresh domain started at `Long.MIN_VALUE`, so every subsequent request went to it until its counter caught up with the existing upstreams. `getWeight`/warmup were never consulted on this path. The reproduction from the issue (100 selections over `a,b`, then 50 over `a,b,c`) returns `50` on current master. ## Changes **`shenyu-loadbalancer`** - `LeastActiveLoadBalance` now reads `Upstream#getInflight()` — the same counter the `p2c` balancer already uses — instead of a private per-domain map. The least active upstream wins; when several share the lowest count they are chosen by weight, warmup included. The `countMap`, its recycling pass and the `ActiveCount` class are removed. - `UpstreamIdentity.leastActiveKey()` is removed, since only the deleted map used it. Identity-based accounting also had a subtle divergence: two upstreams with the same address but different `group`/`version` were tracked separately by the old map, and the per-`Upstream` inflight counter preserves that by construction (`Upstream` instances are per-identity in `UpstreamCacheManager`). **`shenyu-plugin-divide`** - `DividePlugin` reserves the selected upstream for `leastActive` and releases it in `doFinally`, so success, error and cancellation all release the accounting. The half-open probe path reuses the same counter for `p2c` already and needs no extra reservation here. **`shenyu-plugin-httpclient`** - `DefaultRetryStrategy#resend` does the same for a failover attempt, so a retried request is not invisible to the balancer while it runs. ## Observable behavior changes - `leastActive` now distributes by active requests and weight. Traffic proportions between equally idle upstreams follow weight/warmup instead of strict round-robin alternation. That is the documented contract of least-active, but it is a visible change for a selector that relied on the old alternation. - Upstreams that call `LeastActiveLoadBalance` but do not maintain `inflight` (websocket, dubbo/grpc gray routing, and any external caller of `LoadBalanceData`) see every upstream at the same baseline (`1`, the `Upstream` default) and fall back to weight. They were previously receiving the cumulative-selection behavior, which was not useful for them either; making those paths maintain the counter is out of scope here and tracked separately. - `UpstreamIdentity.leastActiveKey()` is removed from the public surface of `shenyu-loadbalancer`. It was added with the old implementation and has no other caller in the repository. ## Validation - `./mvnw -pl shenyu-loadbalancer install` — 70 tests, 0 failures. - `./mvnw -pl shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-divide test` — 66 tests, 0 failures. - `./mvnw -pl shenyu-plugin/shenyu-plugin-httpclient test -Dtest=RetryStrategyTest` — 25 tests, 0 failures. - `./mvnw -pl shenyu-loadbalancer,shenyu-plugin/shenyu-plugin-httpclient,shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-divide checkstyle:check` — passes. Regression tests were verified against the unmodified code: - `LeastActiveLoadBalanceTest` — 5 of the 7 tests fail on current master, including the issue's reproduction (`the new upstream was selected 50 out of 50 times`) and a long-running request whose earlier completed selections mask it (`expected pro.jd.com but was baidu.com`). They pass with the fix. - `DividePluginTest#leastActiveInflightShouldBeReleasedOnCancellation` — fails on current master (`expected 3 but was 2`), passes with the fix. - `RetryStrategyTest#testFailoverReservesAndReleasesLeastActiveAccounting` — fails on current master (`expected 2 but was 1`), passes with the fix. ## Not covered here - Websocket and dubbo/grpc gray routing still call the balancer without maintaining `inflight`; they now fall back to weight instead of the previous cumulative-selection behavior. Extending the counter to those paths is a separate change. - No integration/e2e coverage was added; the regression is fully expressed in the unit tests above. -- 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]
