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

Notify parent ClientConn to re-resolve in grpclb #1699

Merged
merged 3 commits into from
Dec 18, 2017

Conversation

menghanl
Copy link
Contributor

The parent ClientConn should re-resolve when grpclb loses connection to the
remote balancer.

When the ClientConn inside grpclb gets a TransientFailure, it calls
lbManualResolver.ResolveNow(), which calls parent ClientConn's ResolveNow, and
eventually results in re-resolve happening in parent ClientConn's resolver (DNS
for example).

This PR adds a method to balancer.ClientConn interface, so balancer can tell
parent ClientConn to re-resolve.

Copy link
Contributor

@MakMukhi MakMukhi left a comment

Choose a reason for hiding this comment

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

Minor nits, otherwise, looks good.

@@ -96,6 +96,9 @@ type ClientConn interface {
// on the new picker to pick new SubConn.
UpdateBalancerState(s connectivity.State, p Picker)

// ResolveNow is called by balancer to notify gRPC to do a name resolveing.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/resolveing/resolving/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

grpclb_util.go Outdated
ccb balancer.ClientConn
}

func (r *lbManualResolver) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOption) (resolver.Resolver, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since target and opts are not used in the method, maybe we should not name those parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dfawley dfawley assigned menghanl and unassigned MakMukhi Dec 14, 2017
Copy link
Contributor Author

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

Thanks for the review.
All done. PTAL.

@@ -96,6 +96,9 @@ type ClientConn interface {
// on the new picker to pick new SubConn.
UpdateBalancerState(s connectivity.State, p Picker)

// ResolveNow is called by balancer to notify gRPC to do a name resolveing.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

grpclb_util.go Outdated
ccb balancer.ClientConn
}

func (r *lbManualResolver) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOption) (resolver.Resolver, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@menghanl menghanl merged commit 0547980 into grpc:master Dec 18, 2017
@menghanl menghanl deleted the grpclb_resolve_now branch December 18, 2017 23:36
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants