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

api/v3rpc: log grpc stream send/recv errors in server-side #8939

Merged
merged 1 commit into from
Nov 30, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Nov 30, 2017

Helpful for debugging #8904.

c.f. grpc/grpc-go#1687

Copy link
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

lgtm

Out of curiosity. When do we prefer emitting a warning like this versus expanding the error with more information, e.g. return fmt.Errorf("failed to recieve...: %s", err) ?

@gyuho
Copy link
Contributor Author

gyuho commented Nov 30, 2017

@jpbetz

pb.Watch_WatchServer.Send/Recv is generated code. And https://github.com/coreos/etcd/blob/b21180d198f6c87d711980686af388f71c665126/etcdserver/api/v3rpc/watch.go#L111 is only called via https://github.com/coreos/etcd/blob/b21180d198f6c87d711980686af388f71c665126/etcdserver/etcdserverpb/rpc.pb.go#L3479-L3481

Those errors are being returned to gRPC-side, so we can only see those errors when gRPC error log is enabled--now disabled by default (#8810), since it was getting too verbose with other gRPC internal logging.

This etcd-side log will help us verify the fix proposed in grpc/grpc-go#1687.

I will also backport this to 3.2 as well.

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
@gyuho gyuho merged commit 7a1deaa into etcd-io:master Nov 30, 2017
@gyuho gyuho deleted the server-stream-error-log branch November 30, 2017 01:35
gyuho added a commit that referenced this pull request Dec 4, 2017
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.

2 participants