interop/orcalb: Delegate SubConn management to pick_first#8914
interop/orcalb: Delegate SubConn management to pick_first#8914zarinn3pal wants to merge 2 commits intogrpc:masterfrom
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request successfully transitions the orcalb interop balancer to delegate SubConn management to endpointsharding and pick_first, resolving the dualstack support issue (#8809) and adhering to gRFC A61. The implementation of the wrapper balancer and the picker logic for ORCA load report handling is solid, and the new tests provide good coverage for failover and OOB reporting scenarios. I identified one potential nil pointer dereference during balancer shutdown that should be addressed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8914 +/- ##
==========================================
- Coverage 83.04% 83.01% -0.04%
==========================================
Files 411 411
Lines 32892 32892
==========================================
- Hits 27316 27306 -10
- Misses 4181 4189 +8
- Partials 1395 1397 +2 🚀 New features to boost your workflow:
|
Pranjali-2501
left a comment
There was a problem hiding this comment.
Hi @zarinn3pal, thankyou for your contribution.
I have some comments, could you please take a look at that.
65d6901 to
0d4bf8b
Compare
|
Hi @Pranjali-2501 , |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant architectural improvement for the interop ORCA test load balancer. It refactors the balancer to delegate SubConn management to endpointsharding and pick_first, aligning with gRFC A61 and enabling proper dual-stack support. The implementation correctly follows the parent/child balancer pattern, intercepting calls to inject ORCA-specific logic for OOB listeners and picker wrapping. The changes are robust and well-structured. Furthermore, the addition of a comprehensive test suite in orcalb_test.go is excellent, covering various scenarios including multiple addresses, multiple endpoints, and resolver updates. The code quality is high, and I did not find any issues of medium or higher severity.
Pranjali-2501
left a comment
There was a problem hiding this comment.
Added few more comments, PTAL.
0d4bf8b to
5b26eb2
Compare
|
Hi @Pranjali-2501 , |
5b26eb2 to
70513d4
Compare
|
One of the newly added test is failing. PTAL. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ORCA test load balancer to delegate SubConn management to endpointsharding and pick_first, which aligns with gRFC A61. The changes are well-structured and the new implementation is much cleaner and follows modern gRPC load balancing patterns. The addition of comprehensive tests in interop/orcalb_test.go is excellent and covers various important scenarios. I've found a couple of potential race conditions during shutdown that could lead to panics. My review includes suggestions to fix these issues.
24fc540 to
d28bc70
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is an excellent refactoring of the interop ORCA test load balancer. It correctly delegates SubConn management to endpointsharding and pick_first, aligning with gRFC A61. The implementation is robust, intercepting NewSubConn to manage OOB listeners and wrapping the child picker to inject ORCA logic for both OOB and per-RPC reports. The addition of a comprehensive test suite in orcalb_test.go is particularly noteworthy, as it covers various important scenarios including fallback behavior, multiple endpoints, and resolver updates, ensuring the new implementation is well-tested. The overall change significantly improves the architecture and is a high-quality contribution.
d28bc70 to
759c794
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the interop ORCA test load balancer to delegate SubConn management to endpointsharding and pick_first, which is a great architectural improvement aligning with gRFC A61. The implementation is solid and includes a comprehensive new test suite. I have one suggestion to improve the clarity and robustness of the orcaPicker logic.
759c794 to
5377fb9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the interop ORCA test load balancer to delegate SubConn management to endpointsharding and pick_first, aligning with gRFC A61. The changes are well-implemented and include comprehensive tests for the new architecture, covering scenarios with multiple endpoints, multiple addresses, and endpoint updates. My review found one minor issue related to logging consistency, for which I've provided a suggestion. Overall, this is a solid improvement.
5377fb9 to
1f5d68a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the interop ORCA test load balancer to delegate SubConn management to endpointsharding and pick_first, aligning with gRFC A61. This is a significant improvement, moving from a simple single-SubConn model to a more robust one that can handle multiple endpoints and addresses correctly. The changes are well-implemented, and the addition of a comprehensive test suite in interop/orcalb_test.go is excellent for ensuring the new logic is sound. I have one suggestion regarding locking in interop/orcalb.go to improve maintainability and avoid potential deadlocks in the future.
1f5d68a to
ecb5da2
Compare
ecb5da2 to
4ab03de
Compare
This PR switches the interop ORCA test load balancer to delegate SubConn management to endpointsharding + pick_first instead of creating and handling SubConns itself As per gRFC A61 .
Fixes #8809
RELEASE NOTES: N/A