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