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 ttl flag for lock command #8370

Merged
merged 1 commit into from
Aug 9, 2017

Conversation

jiaxuanzhou
Copy link
Contributor

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 ?

@jiaxuanzhou jiaxuanzhou changed the title ADD session ttl for lock cmd #8365 ADD session ttl for lock cmd Aug 5, 2017
Copy link
Contributor

@heyitsanthony heyitsanthony left a 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
Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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")
Copy link
Contributor

Choose a reason for hiding this comment

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

just --ttl?

Copy link
Contributor Author

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

Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

lgtm thanks

@heyitsanthony
Copy link
Contributor

@jiaxuanzhou one last bit: git amend -m "etcdctl: add ttl flag for lock command" so the commit message passes CI style checks, then this should be good to merge. Thanks!

@gyuho gyuho changed the title ADD session ttl for lock cmd etcdctl: add ttl flag for lock command Aug 8, 2017
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@754f454). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8370   +/-   ##
=========================================
  Coverage          ?   76.98%           
=========================================
  Files             ?      353           
  Lines             ?    28115           
  Branches          ?        0           
=========================================
  Hits              ?    21643           
  Misses            ?     4943           
  Partials          ?     1529
Impacted Files Coverage Δ
etcdctl/ctlv3/command/lock_command.go 49.05% <100%> (ø)

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 754f454...9c21eef. Read the comment docs.

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks!

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.

4 participants