-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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 ttl flag for lock command #8370
Conversation
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.
please update etcdctl/README.md
too? thanks!
@@ -28,13 +28,19 @@ import ( | |||
"golang.org/x/net/context" | |||
) | |||
|
|||
var ( | |||
sessionTTL int |
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.
s/sessionTTL/lockTTL since there will probably be a electTTL
eventually
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.
declared lockTTL , removed sessionTTL/defaultSessionTTL
@@ -28,13 +28,19 @@ import ( | |||
"golang.org/x/net/context" | |||
) | |||
|
|||
var ( | |||
sessionTTL int | |||
defaultSessionTTL = 10 |
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.
don't have to declare this, just have lockTTL = 10
?
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.
modified
// NewLockCommand returns the cobra command for "lock". | ||
func NewLockCommand() *cobra.Command { | ||
c := &cobra.Command{ | ||
Use: "lock <lockname> [exec-command arg1 arg2 ...]", | ||
Short: "Acquires a named lock", | ||
Run: lockCommandFunc, | ||
} | ||
c.Flags().IntVarP(&sessionTTL, "session-ttl", "t", defaultSessionTTL, "timeout for session") |
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.
just --ttl
?
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.
modified to --ttl
here is the test:
$ ETCDCTL_API=3 ./etcdctl lock --help
NAME:
lock - Acquires a named lock
USAGE:
etcdctl lock [exec-command arg1 arg2 ...]
OPTIONS:
--ttl=10 timeout for session
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.
lgtm thanks
@jiaxuanzhou one last bit: |
Codecov Report
@@ Coverage Diff @@
## master #8370 +/- ##
=========================================
Coverage ? 76.98%
=========================================
Files ? 353
Lines ? 28115
Branches ? 0
=========================================
Hits ? 21643
Misses ? 4943
Partials ? 1529
Continue to review full report at Codecov.
|
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.
lgtm. Thanks!
1, add session ttl for lock cmd
2, set default session ttl to 10 seconds
@xiang90 @heyitsanthony
please buddy check and suggest if it is a better way to set the ttl as a gloabal flag ?