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.

Sign up for GitHub

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

client: report last connection error to RPCs via v1 balancer API #2508

Merged
merged 1 commit into from
Dec 7, 2018
Merged

client: report last connection error to RPCs via v1 balancer API #2508

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions balancer_v1_wrapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,9 @@ import (
"sync"

"google.golang.org/grpc/balancer"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/status"
)

type balancerWrapperBuilder struct {
Expand Down Expand Up @@ -315,12 +313,12 @@ func (bw *balancerWrapper) Pick(ctx context.Context, opts balancer.PickOptions)
Metadata: a.Metadata,
}]
if !ok && failfast {
Copy link
Member

Choose a reason for hiding this comment

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

@menghanl this is a bug, right? We should return an error regardless of failfast, shouldn't we? Otherwise, we'll be trying to use sc (which will be nil) in the lookup below, and then eventually returning it from the function with no error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Returning nil will not cause error because we check it. nil will result in a re-pick.

We will only get nil here when things are not in sync (between the wrapper and the v1-balancer). This means the connectivity state has changed, and a new pick was (or will be) updated. So I think re-pick makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

But we will also get a re-pick if we return the transient failure error immediately. Technically this works as-is, but if an error happens we should be returning an error, not a nil sc with no error.

Returning a nil sc will also result in the info log (which should probably be a warning) "subconn returned from pick is not *acBalancerWrapper", which shouldn't normally be happening (and it puts our implementation details into the user's log messages).

OK, let's go ahead with this PR but I think we should change this separately. I'll send a PR.

return nil, nil, status.Errorf(codes.Unavailable, "there is no connection available")
return nil, nil, balancer.ErrTransientFailure
}
if s, ok := bw.connSt[sc]; failfast && (!ok || s.s != connectivity.Ready) {
// If the returned sc is not ready and RPC is failfast,
// return error, and this RPC will fail.
return nil, nil, status.Errorf(codes.Unavailable, "there is no connection available")
return nil, nil, balancer.ErrTransientFailure
}
}

Expand Down