Refactor/decouple load balance metrics collection from divide plugin

[hengyuss]

Fixes #6426

Make sure that:

• You have read the contribution guidelines.
• You submit test cases (unit or integration tests) that back your changes.
• Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

[Aias00]

@hengyuss pls fix ci

[Aias00]

I found two issues that should be addressed before merging:

1. SPECIFY_DOMAIN direct-hit requests now depend on load-balancer SPI resolution and request metadata they did not need before.

In DividePlugin, when SPECIFY_DOMAIN matches an upstream, the old path could use that upstream directly. This PR still calls LoadBalancerFactory.getInstance(ruleHandle.getLoadBalance()) and LoadbalancerUtils.buildLoadBalanceData(exchange) afterward (DividePlugin.java lines 99-113, 130-134). That means a blank or unknown load-balance value can now throw from SPI lookup, and a missing remoteAddress can throw from buildLoadBalanceData, even though no load-balancer selection is needed for this path.

Suggested fix: use the same defaulted load-balance value consistently, and avoid building callback state unless callbacks are actually needed. Please add a regression test for a SPECIFY_DOMAIN direct hit.

2. The new LoadBalancer callback API makes load balancing a hidden two-phase protocol.

LoadBalancer.select() is still the public selection contract, but p2c now increments inflight during selection and only decrements/updates it from onSuccess/onError (P2cLoadBalancer.java lines 50-80, 83-107). Only DividePlugin invokes those callbacks; other existing call sites still only call LoadBalancerFactory.selector(...). This makes algorithm correctness depend on every caller knowing to perform the second phase.

Suggested fix: return a per-selection result/handle that owns outcome reporting, or keep this behavior fenced to divide-specific code until all call sites can honor the lifecycle. Also avoid the untyped REQUEST_BEGIN_TIME map dependency in ShortestResponseLoadBalancer if this becomes a generic SPI contract.

Validation I ran locally on PR head 936dec09e:

./mvnw -pl shenyu-loadbalancer,shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-divide -am -Dtest=LoadBalancerFactoryTest,P2cLoadBalancerTest,ShortestResponseLoadBalancerTest,DividePluginTest -DfailIfNoTests=false -DskipITs -DskipRat -Dcheckstyle.skip -Drat.skip=true test

The focused tests passed, but they do not cover the SPECIFY_DOMAIN direct-hit/callback lifecycle cases above.

[hengyuss]

Thanks for the detailed review! I've addressed both issues:

Issue 1 (SPECIFY_DOMAIN): After re-examining the code, I found that the SPECIFY_DOMAIN path on master already works correctly — the newly built Upstream has inflight initialized to 1, so responseTrigger won't cause negative counts. The real problem was that my PR unnecessarily forced the SPECIFY_DOMAIN path through LoadBalancerFactory.getInstance() and buildLoadBalanceData(), which could throw SPI lookup errors or NPE. By reverting to the master pattern, these risks are eliminated.

Issue 2 (Two-phase callback protocol): I agree this was the core problem. I've reverted all changes to the LoadBalancer SPI interface — removed onSuccess/onError default methods, removed LoadBalancerFactory.getInstance(), and kept the callback logic as divide-specific private methods in DividePlugin. This way the SPI contract remains stable and other callers (TCP, gRPC, SDK) are unaffected.

Additional fix: beginTime was an instance variable (private Long beginTime) on master, which is not thread-safe under concurrent requests. I changed it to a local variable and updated successResponseTrigger to accept it as a parameter.
Summary of changes:

• Reverted all LoadBalancer SPI changes (onSuccess/onError, getInstance, REQUEST_BEGIN_TIME in attributes map)
• Kept responseTrigger/successResponseTrigger as private methods in DividePlugin (same as master)
• Fixed beginTime thread-safety: instance variable → local variable + method parameter

[Aias00]

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.

[Aias00]

Good fix — and a real concurrency bug, not just style. beginTime was an instance field on the DividePlugin Spring singleton, so concurrent requests overwrote each other's start time: request B's beginTime = System.currentTimeMillis() would clobber request A's, so A's successResponseTrigger computed its elapsed time against B's start. Moving it to a local captured by the lambda gives each request its own start. The test cleanup (dropping the reflective field set, loadBalancerFactoryMockedStatic.close()) is a nice side effect.