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: put at most once #8335

Merged
merged 4 commits into from
Aug 1, 2017

Conversation

heyitsanthony
Copy link
Contributor

Was able to get errors out of put in 1.4 but the upgrade to 1.5 grpc seems to have changed error handling; haven't investigated fully. Removed the retry and added a test for at-most-once semantics during disconnects.

/cc @HardySimpson

@gyuho
Copy link
Contributor

gyuho commented Jul 31, 2017

get errors out of put in 1.4 but the upgrade to 1.5 grpc

Think this grpc/grpc-go#1369 has changed the behavior? On TransientFailure, it does not close the old transport (does not drain), and creates a new one (https://github.com/grpc/grpc-go/blob/v1.5.x/clientconn.go#L996-L999).

This is the commit with all diffs be794d5?diff=unified#diff-e3c9e37b41869259a878bdb08e61af6cL903.

Was retrying when it shouldn't, causing multiple puts
Switching endpoints on the same client was triggering balancer
reconnect errors that should be tested in clientv3/integration.
Still gets transport closing errors, but no unavailable endpoint errors.
@heyitsanthony
Copy link
Contributor Author

Failing on #6934; merging

@heyitsanthony heyitsanthony merged commit 9f1bfd9 into etcd-io:master Aug 1, 2017
Copy link
Contributor

@HardySimpson HardySimpson left a comment

Choose a reason for hiding this comment

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

cool, distinguish the read and write retry

return err
})
return resp, err
return rkv.KVClient.Put(ctx, in, opts...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good, if grpc return the error like remote network not availabe, I think we should still do retry

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.

None yet

3 participants