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: make sure non-nil done returned by Pick is called #2688

Merged
merged 2 commits into from Mar 19, 2019

Conversation

@menghanl
Copy link
Contributor

menghanl commented Mar 13, 2019

Special case: when SubConn returned by Picker is not Ready, call done before
looping back to re-pick.

Special case: when SubConn returned by Picker is not Ready, call done before
looping back to re-pick.
@@ -165,6 +165,14 @@ func (bp *pickerWrapper) pick(ctx context.Context, failfast bool, opts balancer.
}
return t, done, nil
}
if done != nil {
done(balancer.DoneInfo{
Err: nil,

This comment has been minimized.

Copy link
@dfawley

dfawley Mar 13, 2019

Contributor

These are all the defaults - maybe delete and a comment "default DoneInfo indicates RPC did not occur"?

This comment has been minimized.

Copy link
@menghanl

menghanl Mar 14, 2019

Author Contributor

If the field was named NoBytesSent, we would want to set it to true (It's great that's not the case :) )

Deleted and explain in a comment.

This comment has been minimized.

Copy link
@menghanl

menghanl Mar 14, 2019

Author Contributor

Didn't test this because returning a non-Ready SubConn from picker is very racy.

// Stop server and at the same time send RPCs. There are chances that picker
// is not updated in time, causing a non-Ready SubConn to be returned.
var wg sync.WaitGroup
wg.Add(1)

This comment has been minimized.

Copy link
@dfawley

dfawley Mar 13, 2019

Contributor

The loneliest waitgroup. Maybe use a channel instead?

This comment has been minimized.

Copy link
@menghanl

menghanl Mar 14, 2019

Author Contributor

Done

@menghanl menghanl assigned dfawley and unassigned menghanl Mar 14, 2019
@menghanl menghanl merged commit ce45558 into grpc:master Mar 19, 2019
2 checks passed
2 checks passed
cla/linuxfoundation menghanl authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@menghanl menghanl deleted the menghanl:balancer_always_done branch Mar 19, 2019
@dfawley dfawley added the Type: Bug label Mar 22, 2019
@dfawley dfawley added this to the 1.20 Release milestone Mar 22, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.