Aias00 commented on PR #6429: URL: https://github.com/apache/shenyu/pull/6429#issuecomment-5156776219
Reviewed #6429. The mechanical change is correct and behavior-preserving, but the PR title/`Fixes #6426` oversell the scope, and the changed path has no regression test. **1. should_fix — Scope vs. linked issue.** Body says `Fixes #6426` and the title is "decouple load balance metrics collection from DividePlugin", but only sub-issue #3 (thread-safety of `beginTime`) is addressed. The actual decoupling proposed in #6426 — adding `onSuccess`/`onError` defaults to `LoadBalancer` and moving the strategy callbacks out of `DividePlugin` — is not done: `LoadBalancer.java` is unchanged and `DividePlugin` still hard-codes the P2C/SHORTEST_RESPONSE branches with strategy-specific callbacks (`DividePlugin.java:133-140`). Merging will auto-close #6426 while the SRP/OCP problems (#1, #2) remain. Either rename this to a focused "fix: make shortestResponse beginTime per-request" and switch to `Relates to #6426`, or land the full decoupling. **2. nit — No test for the changed path.** `doExecuteTest` never sets `loadBalance=shortestResponse`, so the only line that changed (`DividePlugin.java:136-139`) is uncovered. `successResponseTriggerTest` invokes the private helper via reflection and only asserts `succeeded == 1` — it would pass even if the caller never wired the local `beginTime` into the `doOnSuccess` lambda. Suggest a `doExecute` test with `loadBalance=shortestResponse`, a `Mono.empty()` chain, asserting `upstream.getSucceeded().get() == 1` and `upstream.getSucceededElapsed().get() > 0` after the `StepVerifier` completes. Behavior-preservation verified against master: metrics still recorded on the same paths (shortestResponse: `doOnSuccess` only; P2C: `doOnSuccess`+`doOnError`), same Upstream counters (`succeeded`/`succeededElapsed`, consumed by `ShortestResponseLoadBalancer`), same capture timing, no double-counting/drop. Promoting `Long beginTime` from a singleton instance field to an effectively-final local `long` is the correct fix for the race and also removes a latent auto-unbox NPE. Only a `private` method signature changed — no SPI/compat impact (`rg` confirms no external consumers/reflection). CI is green. -- 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]
