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

clientv3: health balancer #8545

Merged
merged 4 commits into from
Sep 18, 2017
Merged

Conversation

heyitsanthony
Copy link
Contributor

Works with the watch keepalive test from the grpc ping timeout patch.

Supersedes #8327
Fixes #8326

if err == nil {
return nil
}
if grpc.Code(err) == codes.Unavailable {
Copy link
Contributor

Choose a reason for hiding this comment

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

gRPC is deprecating grpc.Code.

ev, _ := status.FromError(err)
if ev.Code() == codes.Unavailable {

?

@heyitsanthony heyitsanthony force-pushed the health-balancer branch 2 times, most recently from 6020a87 to 149943f Compare September 13, 2017 21:46
cancel()
if err != nil {
if ev, _ := status.FromError(err); ev.Code() == codes.Unavailable &&
grpc.ErrorDesc(err) == "unknown service grpc.health.v1.Health" {
Copy link
Contributor

Choose a reason for hiding this comment

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

define this string as const?

Copy link
Contributor

Choose a reason for hiding this comment

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

s/grpc.ErrorDesc/rpctypes.ErrorDesc?

grpc.ErrorDesc is being deprecated.

@xiang90
Copy link
Contributor

xiang90 commented Sep 14, 2017

lgtm in general. @gyuho or @fanminshi should give this a more close look.

if len(hb.addrs) == 1 || len(hb.unhealthy) == 0 || len(hb.unhealthy) == len(hb.addrs) {
return hbAddrs
}
addrs := make([]grpc.Address, len(hb.addrs)-len(hb.unhealthy))
Copy link
Contributor

Choose a reason for hiding this comment

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

s/make([]grpc.Address, len(hb.addrs)-len(hb.unhealthy))/make([]grpc.Address, 0, len(hb.addrs)-len(hb.unhealthy))

@heyitsanthony heyitsanthony force-pushed the health-balancer branch 2 times, most recently from ff1eb43 to 7b5729a Compare September 15, 2017 21:22
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@6cf0fd7). Click here to learn what that means.
The diff coverage is 72.78%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8545   +/-   ##
=========================================
  Coverage          ?   65.94%           
=========================================
  Files             ?      361           
  Lines             ?    29710           
  Branches          ?        0           
=========================================
  Hits              ?    19592           
  Misses            ?     8451           
  Partials          ?     1667
Impacted Files Coverage Δ
clientv3/retry.go 58.15% <100%> (ø)
clientv3/health_balancer.go 63.8% <63.8%> (ø)
clientv3/balancer.go 91.86% <85.41%> (ø)
clientv3/client.go 67.24% <87.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cf0fd7...49e5e78. Read the comment docs.

@heyitsanthony
Copy link
Contributor Author

got this working; next needed to wait until the connection went down so the retried RPC wouldn't race with the balancer and use the old connection

select {
case b.updateAddrsC <- notifyNext:
case <-b.stopc:
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return on <-b.stopc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the channel is closed, so it'll pass through the next select without blocking

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants