Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

balancer: Enforce embedding requirement for balancer.ClientConn #8026

Merged
merged 5 commits into from
Feb 6, 2025

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Jan 22, 2025

Similar to #7840, this PR ensures gRPC can add new methods to the ClientConn interface. This change prepares us for passing the metrics recorder to LB policies through the balancer.ClientConn interface instead of the balancer.BuildOptions as being done presently. Using balancer.BuildOptions doesn't allow gRPC to ensure users are propagating the metrics recorder to child LB Policies and can lead to nil pointer dereferences at runtime.

RELEASE NOTES:

  • balancer: An internal method is added to the balancer.ClientConn interface to force implementors to embed a delegate implementation. This helps grpc-go add new methods to the interface without breaking builds for implementors of custom LB Policies.

@arjan-bal arjan-bal added Type: API Change Breaking API changes (experimental APIs only!) Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. labels Jan 22, 2025
@arjan-bal arjan-bal added this to the 1.71 Release milestone Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.34%. Comparing base (ee3e8d9) to head (0866eef).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8026      +/-   ##
==========================================
+ Coverage   82.29%   82.34%   +0.04%     
==========================================
  Files         388      387       -1     
  Lines       39028    39037       +9     
==========================================
+ Hits        32120    32144      +24     
+ Misses       5578     5567      -11     
+ Partials     1330     1326       -4     
Files with missing lines Coverage Δ
balancer/balancer.go 96.29% <ø> (ø)
balancer/subconn.go 100.00% <ø> (ø)
balancer_wrapper.go 86.30% <ø> (-1.03%) ⬇️
internal/balancer/gracefulswitch/gracefulswitch.go 86.52% <100.00%> (-0.50%) ⬇️
internal/testutils/balancer.go 82.35% <ø> (ø)
xds/internal/balancer/outlierdetection/balancer.go 88.41% <100.00%> (-0.79%) ⬇️

... and 36 files with indirect coverage changes

@dfawley
Copy link
Member

dfawley commented Jan 23, 2025

For now, this is blocked on at least one dependency being updated.

@arjan-bal
Copy link
Contributor Author

Merged a PR to avoid breakages in grpc-gcp-go: GoogleCloudPlatform/grpc-gcp-go#91

@dfawley dfawley assigned arjan-bal and unassigned dfawley Jan 24, 2025
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure the GCP balancer changes have been released, updated in the google cloud client libraries repo & released there ( 😢 ), and imported before this is merged.

@@ -293,6 +294,7 @@ func (gsb *Balancer) Close() {
// State updates from the wrapped balancer can result in invocation of the
// graceful switch logic.
type balancerWrapper struct {
balancer.ClientConn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are embedding now, we can remove the Target implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@arjan-bal
Copy link
Contributor Author

@arjan-bal arjan-bal merged commit 3e27c17 into grpc:master Feb 6, 2025
15 checks passed
janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. Type: API Change Breaking API changes (experimental APIs only!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants