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

etcdctl: add timeout to snapshot save command #10301

Merged

Conversation

jingyih
Copy link
Contributor

@jingyih jingyih commented Dec 5, 2018

Snapshot save command by default has no timeout, but should respect user specified command timeout.

Fix #10298

/cc @jpbetz @gyuho @xiang90

} else {
// if user does not specify "--command-timeout" flag, there will be no timeout for snapshot save command
ctx, cancel = context.WithCancel(context.Background())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like you could condense this a bit.

ctx, cancel := context.WithCancel(context.Background())
if ifCommandTimeoutFlagSet(cmd) {
	ctx, cancel = commandCtx(cmd)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@@ -82,6 +82,14 @@ func commandCtx(cmd *cobra.Command) (context.Context, context.CancelFunc) {
return context.WithTimeout(context.Background(), timeOut)
}

func ifCommandTimeoutFlagSet(cmd *cobra.Command) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

isCommandTimeoutFlagSet

func ifCommandTimeoutFlagSet(cmd *cobra.Command) bool {
commandTimeoutFlag := cmd.Flags().Lookup("command-timeout")
if commandTimeoutFlag == nil {
ExitWithError(ExitError, fmt.Errorf("expect command-timeout flag to exist"))
Copy link
Contributor

Choose a reason for hiding this comment

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

probably we should panic here. this is an internal error.

@xiang90
Copy link
Contributor

xiang90 commented Dec 5, 2018

lgtm after fixing the issues in the comments.

@codecov-io
Copy link

Codecov Report

Merging #10301 into master will decrease coverage by 0.28%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10301      +/-   ##
==========================================
- Coverage   71.93%   71.65%   -0.29%     
==========================================
  Files         391      391              
  Lines       36386    36400      +14     
==========================================
- Hits        26176    26084      -92     
- Misses       8414     8495      +81     
- Partials     1796     1821      +25
Impacted Files Coverage Δ
etcdctl/ctlv3/command/util.go 19.75% <60%> (+2.64%) ⬆️
etcdctl/ctlv3/command/snapshot_command.go 71.71% <60%> (-0.51%) ⬇️
proxy/grpcproxy/register.go 69.44% <0%> (-13.89%) ⬇️
pkg/fileutil/purge.go 65.9% <0%> (-6.82%) ⬇️
clientv3/leasing/util.go 91.66% <0%> (-6.67%) ⬇️
etcdctl/ctlv3/command/lease_command.go 65.34% <0%> (-5.95%) ⬇️
clientv3/leasing/cache.go 87.77% <0%> (-5%) ⬇️
proxy/grpcproxy/watcher.go 89.79% <0%> (-4.09%) ⬇️
pkg/transport/listener.go 58.67% <0%> (-4.09%) ⬇️
etcdserver/util.go 95% <0%> (-3.75%) ⬇️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f450bf...bfcb5d3. Read the comment docs.

snapshot save command by default has no timeout, but respects user
specified command timeout.
@jingyih jingyih force-pushed the add_timeout_to_etcdctl_snapshot_save branch from bfcb5d3 to 5b6b03d Compare December 5, 2018 04:38
@jingyih
Copy link
Contributor Author

jingyih commented Dec 5, 2018

@xiang90 Thanks for the review. All fixed.

@gyuho
Copy link
Contributor

gyuho commented Dec 5, 2018

@jingyih
Copy link
Contributor Author

jingyih commented Dec 5, 2018

@gyuho Sure. Thanks for reminding:)

jingyih added a commit to jingyih/etcd that referenced this pull request Dec 5, 2018
jingyih added a commit to jingyih/etcd that referenced this pull request Dec 5, 2018
xiang90 added a commit that referenced this pull request Dec 5, 2018
@xiang90 xiang90 merged commit 3f01426 into etcd-io:master Dec 5, 2018
@jingyih jingyih deleted the add_timeout_to_etcdctl_snapshot_save branch September 7, 2019 07:08
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.

5 participants