Wubabalala opened a new pull request, #16146:
URL: https://github.com/apache/dubbo/pull/16146
## What is the purpose of the change?
Fixes two bugs in `AdaptiveMetrics.getLoad()` that cause adaptive load
balancing to degrade to near-random distribution under normal traffic.
Fixes #15810
## Root Cause Analysis
### Bug 1: Penalty fires on every normal response
`setProviderMetrics()` sets both `currentProviderTime` and `currentTime` to
`serviceTime` (L93-94). The penalty check `currentProviderTime == currentTime`
(L58) is therefore always true after any normal response. This overwrites real
RT with `timeout * 2L`, making a 10ms server indistinguishable from a 200ms
server.
**Code trace:**
1. `AdaptiveLoadBalanceFilter.onResponse()` → calls `setProviderMetrics()`
with real RT
2. `setProviderMetrics()` L93-94: `currentProviderTime = serviceTime;
currentTime = serviceTime;` (same value)
3. Next `getLoad()` L58: `currentProviderTime == currentTime` → always true
→ L60: `lastLatency = timeout * 2L`
4. Real RT overwritten. EWMA fed garbage data.
### Bug 2: Decay to zero
`lastLatency >> multiple` (L62) has no floor. After 2s idle with
timeout=100ms: `multiple=21`, `200 >> 21 = 0`. The slowest server appears
"fastest" and gets flooded with traffic.
## What is changed?
1. **Added `volatile boolean providerUpdated` flag** — a coalescing
freshness signal (not an event counter). `setProviderMetrics()` sets it to
`true`; `getLoad()` checks it: if true, uses real `lastLatency` (already set by
`setProviderMetrics`); if false, applies decay with floor.
2. **Removed the broken equality-based penalty trigger** — replaced
`currentProviderTime == currentTime` with explicit freshness tracking via the
flag. The original equality check was intended to detect stale metrics but
fires on the opposite condition (fresh metrics).
3. **Decay floor: `Math.max(1L, timeout / 100L)`** — scales with service
timeout. A 5000ms-timeout service gets a 50ms floor; a 100ms-timeout service
gets a 1ms floor.
4. **New `AdaptiveMetricsTest`** — 6 test cases through public API (no
reflection, no `Thread.sleep`):
- `freshMetrics_usesRealLatency_notPenalty` — Bug 1 regression
- `decayWithoutUpdate_doesNotReachZero` — Bug 2 regression
- `multipleServers_fastServerPreferred` — end-to-end ordering
- `degradedServer_loadIncreases` — degradation detection
- `outOfOrderServiceTime_isDiscarded` — guard: stale timestamp rejected
- `forcedPick_notBroken` — guard: pickTime starvation prevention intact
## Concurrency note
`providerUpdated` is `volatile` because `setProviderMetrics()` runs in an
async single-thread executor (`AdaptiveLoadBalanceFilter.getExecutor()`) while
`getLoad()` runs in the caller thread. The flag is a coalescing signal — if
multiple `setProviderMetrics()` calls occur between two `getLoad()` calls,
intermediate updates are coalesced. This is acceptable because `lastLatency`
and `ewma` are already updated to the latest values in each
`setProviderMetrics()` call; the flag only indicates "fresh data exists since
last load calculation".
## Why this approach over PR #16048?
| Aspect | #16048 | This PR |
|--------|--------|---------|
| Penalty fix | `==` → `>` (penalty never fires) | Explicit freshness flag
(correct semantics) |
| Decay floor | `Math.max(1L, ...)` (arbitrary) | `Math.max(1L,
timeout/100L)` (scales with timeout) |
| Tests | Reflection + Thread.sleep | Public API only, deterministic |
| Root cause | Symptom-level condition flip | Addresses the design flaw
(implicit → explicit freshness) |
## Simulation results
Python simulation comparing buggy vs fixed (faithful port of
`AdaptiveMetrics.java`):
```
BUGGY: fast(10ms) ewma=105.0 medium(50ms) ewma=125.0 slow(200ms)
ewma=199.9
FIXED: fast(10ms) ewma=10.0 medium(50ms) ewma=50.0 slow(200ms)
ewma=199.9
```
After fix, EWMA accurately reflects real RT. Degradation detection and decay
floor also verified. Full analysis and scripts:
https://github.com/apache/dubbo/pull/16048#issuecomment-4087574069
## Verifying this change
```bash
mvn -pl dubbo-rpc/dubbo-rpc-api -am test -Dtest=AdaptiveMetricsTest
mvn -pl dubbo-cluster -am test -Dtest=AdaptiveLoadBalanceTest
```
## Checklist
- [x] Related GitHub issue referenced (Fixes #15810)
- [x] Logic change explained clearly
- [x] Unit tests added (6 cases)
- [x] Concurrency implications documented
- [x] No changes to public API surface
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
