-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: change roundrobin to accept empty address list #3491
Conversation
… healthy endpoints TODO empty locality should result in transient failure. Needs to fix RoundRobin tests: - if a locality contains no endpoints - if this is the first update - if this is not the first update
for src := cc.GetState(); src == connectivity.Ready; src = cc.GetState() { | ||
if !cc.WaitForStateChange(ctx, src) { | ||
t.Fatalf("timed out waiting for state change. got %v; want !%v", src, connectivity.Ready) | ||
ctx2, cancel2 := context.WithTimeout(context.Background(), 50*time.Millisecond) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this enough time? a few seconds should be fine before detecting the failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 0
const msgWant = "produced zero addresses" | ||
if _, err := testc.EmptyCall(ctx, &testpb.Empty{}); err == nil || !strings.Contains(status.Convert(err).Message(), msgWant) { | ||
t.Fatalf("EmptyCall() = _, %v, want _, Contains(Message(), %q)", err, msgWant) | ||
if _, err := testc.EmptyCall(ctx2, &testpb.Empty{}, grpc.WaitForReady(true)); status.Code(err) != codes.DeadlineExceeded { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test the error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot. The balancer no longer sets the error. This will be a pure deadline exceeded error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can. This is done now.
We won't start to report "zero addresses" immediately, but will when all the connections are down.
The previous change to combine resolver errors with transient errors still makes sense here.
p1 := <-cc.newPickerCh | ||
want := []balancer.SubConn{sc1} | ||
if err := isRoundRobin(want, subConnFromPicker(p1)); err != nil { | ||
// t.Fatalf("want %v, got %v", want, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. (There are a lot of them..)
Roundrobin will remove all SubConns. The ClientConn will set SubConn state change to shutdown, and the overall state will turn transient failure.
Roundrobin will remove all SubConns. The ClientConn will set SubConn state change to shutdown, and the overall state will turn transient failure.