Aias00 commented on PR #6429: URL: https://github.com/apache/shenyu/pull/6429#issuecomment-5138233022
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`: ```bash ./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. -- 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]
