-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
etcdctl/ctlv2: total-timeout for Sync #5310
Conversation
@@ -156,7 +156,12 @@ func getTransport(c *cli.Context) (*http.Transport, error) { | |||
CertFile: certfile, | |||
KeyFile: keyfile, | |||
} | |||
return transport.NewTransport(tls, defaultDialTimeout) | |||
|
|||
to := c.GlobalDuration("total-timeout") |
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.
why shall we do this? say if the total-timeout is 10 seconds and the defaultDialTimeout is 5 seconds, the change might cause the dailtimeout to be rewrote by a longer one which is 10 seconds. this is incorrect.
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.
if totalTimeout !=0 && to < defaultDialTimeout {
dialTimeout = totalTimeout
}
/cc @gyuho
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.
@xiang90 Did you mean
to := c.GlobalDuration("total-timeout")
if to != 0 && to > defaultDialTimeout {
defaultDialTimeout = to
}
return transport.NewTransport(tls, defaultDialTimeout)
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.
Oh I meant:
to := c.GlobalDuration("total-timeout")
if to != 0 && to < defaultDialTimeout {
defaultDialTimeout = to
}
return transport.NewTransport(tls, defaultDialTimeout)
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.
no.
Your code has two issues:
- we should never modify the default variable in any case.
- we should keep the shorter timeout not longer. Say I want the total timeout to be 5 seconds, but the defaultDialtimeout is 30 seconds. I should give the transport a timeout of 5 seconds, not 30 seconds. Or dial might block for 30 seconds, which is longer than 5 seconds.
dailTimeout := defaultDialTimeout
totalTimeout := c.GlobalDuration("total-timeout")
if totalTimoeout != 0 && totalTimeout < defaultDialTimeout {
defaultDialTimeout = totalTimeout
}
return transport.NewTransport(tls, defaultDialTimeout)
@xiang90 You are right. Just reverted it. PTAL. Thanks. |
@@ -203,7 +203,7 @@ func mustNewClient(c *cli.Context) client.Client { | |||
if debug { | |||
fmt.Fprintf(os.Stderr, "start to sync cluster using endpoints(%s)\n", strings.Join(hc.Endpoints(), ",")) | |||
} | |||
ctx, cancel := context.WithTimeout(context.Background(), client.DefaultRequestTimeout) | |||
ctx, cancel := contextWithTotalTimeout(c) |
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.
also here, we should also add a timeout for the request timeout.
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.
also here, we should also add a timeout for the request timeout.
Sorry, can you explain more? This already puts timeout for the command:
func contextWithTotalTimeout(c *cli.Context) (context.Context, context.CancelFunc) {
return context.WithTimeout(context.Background(), c.GlobalDuration("total-timeout"))
}
c.GlobalDuration("timeout")
is already used for HeaderTimeoutPerRequest
here https://github.com/coreos/etcd/blob/master/etcdctl/ctlv2/command/util.go#L302
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.
Oh. I am not aware of https://github.com/coreos/etcd/blob/master/etcdctl/ctlv2/command/util.go#L302.
Then this is fine. Thanks.
Can you give it a try after the change? Does this fix the problem mentioned in #4897 |
@xiang90 Yeah, just pushed a change to address your last comment and tried
It times out after 5 seconds (not 30 seconds) |
lgtm |
Thanks. Merging in greens. |
Fix #4897.