interop/orcalb: Delegate SubConn management to pick_first#8914
interop/orcalb: Delegate SubConn management to pick_first#8914zarinn3pal wants to merge 5 commits intogrpc:masterfrom
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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% 81.82% -1.22%
==========================================
Files 411 413 +2
Lines 32892 33427 +535
==========================================
+ Hits 27316 27353 +37
- Misses 4181 4268 +87
- Partials 1395 1806 +411 🚀 New features to boost your workflow:
|
Pranjali-2501
left a comment
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.
zarinn3pal
force-pushed
the
feat/interop-orca-to-pf
branch
from
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
Pranjali-2501
left a comment
There was a problem hiding this comment.
Added few more comments, PTAL.
zarinn3pal
force-pushed
the
feat/interop-orca-to-pf
branch
from
0d4bf8b to
5b26eb2
Compare
|
Can you please not mark the comments as resolved. That is typically done by the person who starts that comment. Now, if I have to know what comments I made and what to replied to them, I have to unresolve every single comment. |
…s for map lookups, improved readability of hasMetrics condition, and replaced sync.Map with sync.Mutex
|
Hi @easwars , |
| "testing" | ||
| "time" | ||
|
|
||
| v3orcapb "github.com/cncf/xds/go/xds/data/orca/v3" |
There was a problem hiding this comment.
Please group proto imports in a separate import block. See: https://google.github.io/styleguide/go/decisions#import-grouping
| "google.golang.org/grpc/internal" | ||
| "google.golang.org/grpc/internal/grpctest" | ||
| "google.golang.org/grpc/internal/stubserver" | ||
| testgrpc "google.golang.org/grpc/interop/grpc_testing" |
There was a problem hiding this comment.
This needs to be imported twice, once as testgrpc and once as testpb. Services and methods needs to be used from the former while messages need to used from the latter. Please see any test file in our repo to get an idea of how this is done.
|
Hi @easwars , |
I think I've addressed the two questions that I saw. Please take care of the other comments. |
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
|
@easwars , I have addressed the remaining comments. And added why using async would not be a good idea. |
|
But the test is still failing on 32 bit architecture. Can you please take a look? |
5 participants
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