-
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: use user specified timeout value for entire command execution #3530
Conversation
e86b95f
to
4ec71d9
Compare
I don't like current approach because it introduces new parameter in each function call, and it has one more level indirection. Could we use |
@yichengq thanks for your review. However, current approach can reduce possibility of using wrong timeout value for each context (the problem this PR is trying to fix). So I thought introducing a new wrapper function actionWithTotalTimeout() is valuable. And commands which doesn't require total timeout aren't changed. Of course, if you prefer declaration of ctx in each function, I'll pick your approach. |
I still prefer to declare ctx. Current solution makes the code hard to read, and the indirection could be replaced by one-line declaration.
We could add a function to return overall context and its cancel.
I think other commands need to have total timeout too. They are using context.TODO now. |
@yichengq ok, I'll change this PR based on your policy later |
82407bb
to
b4847d4
Compare
@yichengq updated based on the policy |
@@ -86,9 +86,8 @@ func actionMemberAdd(c *cli.Context) { | |||
mAPI := mustNewMembersAPI(c) | |||
|
|||
url := args[1] | |||
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.
I would defer cancel()
here. It is reasonable because the ctx is for the whole function.
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.
defer cancel()
seems nice, I'll fix it.
b4847d4
to
1c7da6b
Compare
@yichengq updated based on your comments |
1c7da6b
to
48c4846
Compare
updated the commit log a little bit |
totalCtx, totalCancel := contextWithTotalTimeout(c) | ||
defer totalCancel() | ||
|
||
ctx, cancel := contextWithPerRequestTimeout(c, totalCtx) |
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.
The per request timeout
is used at https://github.com/coreos/etcd/blob/master/etcdctl/command/util.go#L251. It is the timeout for each HTTP request(ref: http://godoc.org/github.com/coreos/etcd/client#Config). When we call mAPI.Add, it may include several HTTP requests, so we cannot set per request timeout
as its context.
It is fine that we just use totalCtx for all requests in the this function IMO.
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.
Yes, I misused client.DefaultRequestTimeout
:( Thanks for pointing. I'll fix it.
48c4846
to
2f17c03
Compare
@yichengq updated for using correct context and timeout |
etcdctl should be capable to use a user specified timeout value for total command execution, not only per request timeout. This commit adds a new option --total-timeout to the command. The value passed via this option is used as a timeout value of entire command execution. Fixes coreos#3517
2f17c03
to
8ebc933
Compare
LGTM. Do you have plan to fix the left context.Background() ones? For example, https://github.com/coreos/etcd/blob/master/etcdctl/command/set_command.go#L58 |
@yichengq thanks for your review. Sure, I'd like to work on other parts. Should I fix them in this PR? |
No. this one is good enough. Thanks for the contribution! ❤️ |
etcdctl: use user specified timeout value for entire command execution
etcdctl should be capable to use a user specified timeout value for
total command execution, not only per request timeout. This commit
adds a new option --total-timeout to the command. The value passed via
this option is used as a timeout value of entire command execution.
Fixes coreos#3517
This is the new version of the PR #3518 . This new one is so different from the old one, so I made a new PR.