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

balancergroup: Propagate balancer.BuildOptions to child policies #4184

Merged
merged 2 commits into from
Feb 8, 2021

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Feb 4, 2021

API changes:

  • balancergroup.New() is modified to take balancer.BuildOptions as its second argument.
  • This balancer.BuildOptions is passed to child policies when they are constructed.
  • Since a single parent balancer uses the balancergroup, it is better to take the balancer.BuildOptions as part of New() instead of as part of Add().

Other changes:

  • Update clustermanager, eds and weightedtarget LB policies to pass balancer.BuildOptions to the balancergroup.
  • Add unit tests to make sure the balancer.BuildOptions are propagated properly.
  • Get rid of testutils.TestConstBalancerBuilder and use stub.Balancer instead.

Fixes #4185

@@ -590,16 +594,20 @@ func (s) TestBalancerGroup_start_close(t *testing.T) {
// whenever it gets an address update. It's expected that start() doesn't block
// because of deadlock.
func (s) TestBalancerGroup_start_close_deadlock(t *testing.T) {
const balancerName = "stub-TestBalancerGroup_start_close_deadlock"
stub.Register(balancerName, stub.BalancerFuncs{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is to export stub.bb, and add a NewBB(). So you don't need to register and get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will send a separate PR for that since I would have to touch other existing call sites as well. Thanks.


// Send an empty clientConn state change. This should trigger the
// verification of the buildOptions being passed to the child policy.
bg.UpdateClientConnState(testBalancerIDs[0], balancer.ClientConnState{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this function not return the error from the stub?
If it does, we don't need ccsCh, right?


I think balancergroup calls the methods inline.

Unlike e.g. cluster_manager, where we get errors from bg.UpdateClientConnState, but we don't aggregate and return that error. So ccsCh would be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch. Done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xds: Pass balancer.BuildOptions to child policies from parent
2 participants