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: fix health balancer, retry #8688

Closed
wants to merge 15 commits into from
Closed

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Oct 11, 2017

c.f. efd7800

@gyuho gyuho added the WIP label Oct 12, 2017
@xiang90
Copy link
Contributor

xiang90 commented Oct 12, 2017

why this is still wip?

@@ -473,8 +473,8 @@ func TestKVNewAfterClose(t *testing.T) {

donec := make(chan struct{})
go func() {
if _, err := cli.Get(context.TODO(), "foo"); err != context.Canceled {
t.Fatalf("expected %v, got %v", context.Canceled, err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is added because previously, no matter what, retry.go switches endpoint (resets connection) so didn't hit grpc.ErrClientConnClosing path. Now, only does switch when it's transient error, so grpc.ErrClientConnClosing is expected when concurrent goroutine closes client.

@gyuho gyuho force-pushed the transient branch 2 times, most recently from 68be468 to a56d5f4 Compare October 12, 2017 19:47
@gyuho gyuho changed the title clientv3: do not retry on non-transient server error clientv3/retry: fix rpctypes error handling Oct 12, 2017
@gyuho gyuho changed the title clientv3/retry: fix rpctypes error handling clientv3/retry: fix error handling Oct 13, 2017
@gyuho gyuho changed the title clientv3/retry: fix error handling clientv3: fix retrial logic, error handling Oct 13, 2017
@gyuho gyuho requested a review from xiang90 October 13, 2017 23:12
@gyuho gyuho removed the WIP label Oct 13, 2017
@@ -181,7 +190,7 @@ type retryLeaseClient struct {
func RetryLeaseClient(c *Client) pb.LeaseClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

comment needs to be fixed.

@@ -181,7 +190,7 @@ type retryLeaseClient struct {
func RetryLeaseClient(c *Client) pb.LeaseClient {
retry := &retryLeaseClient{
pb.NewLeaseClient(c.conn),
c.newRetryWrapper(isReadStopError),
c.newRetryWrapper(false),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is incorrect. leaseGrant is write, but list leases is read.

type retryClusterClient struct {
pb.ClusterClient
retryf retryRpcFunc
}

// RetryClusterClient implements a ClusterClient that uses the client's FailFast retry policy.
func RetryClusterClient(c *Client) pb.ClusterClient {
return &retryClusterClient{pb.NewClusterClient(c.conn), c.newRetryWrapper(isWriteStopError)}
return &retryClusterClient{pb.NewClusterClient(c.conn), c.newRetryWrapper(true)}
Copy link
Contributor

Choose a reason for hiding this comment

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

not correct. memerlist is read.

@xiang90
Copy link
Contributor

xiang90 commented Oct 14, 2017

we need to make the wrapper to understand read/write type per RPC, not per Service. A service might have both read/write type RPCs.

@gyuho gyuho force-pushed the transient branch 4 times, most recently from d915681 to b0dee09 Compare October 14, 2017 21:16
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Set grpc.FailFast==true, so that RPCs fail immediately on broken
connections or unreachable servers. And let client balancer handle
the retries.

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
…erClose

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
@gyuho gyuho force-pushed the transient branch 4 times, most recently from 9396c35 to db175aa Compare October 15, 2017 00:07
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
@gyuho gyuho changed the title clientv3: fix retrial logic, error handling clientv3: fix health balancer, retry Oct 16, 2017
@gyuho gyuho closed this Oct 17, 2017
@gyuho gyuho deleted the transient branch October 17, 2017 18:21
@etcd-io etcd-io deleted a comment from codecov-io Oct 18, 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