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/v3: enable 'require-leader' for 'watch' command #8672

Merged
merged 1 commit into from
Oct 9, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Oct 9, 2017

Address #7941 (comment).

@xiang90
Copy link
Contributor

xiang90 commented Oct 9, 2017

i think the require leader should probably be enabled for ctl tool.

@xiang90
Copy link
Contributor

xiang90 commented Oct 9, 2017

so no need the flag.

@gyuho
Copy link
Contributor Author

gyuho commented Oct 9, 2017

@xiang90

should probably be enabled for ctl tool.

You mean by default, enabled for watch?

@xiang90
Copy link
Contributor

xiang90 commented Oct 9, 2017

yes, enable by default for watch. etcdctl users would expect the command to be always interactive. it is probably the best option to simply enable require leader and do not provide that as a flag.

To help with network partition cases.

Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
@gyuho gyuho changed the title etcdctl/ctlv3: add 'require-leader' flag etcdctl/ctlv3: enable 'require-leader' for 'watch' command Oct 9, 2017
@gyuho
Copy link
Contributor Author

gyuho commented Oct 9, 2017

@xiang90 PTAL.

@xiang90
Copy link
Contributor

xiang90 commented Oct 9, 2017

lgtm

@gyuho gyuho merged commit e47be1f into etcd-io:master Oct 9, 2017
@gyuho gyuho deleted the require-leader branch October 9, 2017 20:38
@gyuho gyuho changed the title etcdctl/ctlv3: enable 'require-leader' for 'watch' command etcdctl/v3: enable 'require-leader' for 'watch' command Oct 27, 2017
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8672   +/-   ##
=========================================
  Coverage          ?   76.01%           
=========================================
  Files             ?      360           
  Lines             ?    29643           
  Branches          ?        0           
=========================================
  Hits              ?    22533           
  Misses            ?     5544           
  Partials          ?     1566
Impacted Files Coverage Δ
clientv3/client.go 84.32% <ø> (ø)
etcdserver/api/v3rpc/rpctypes/error.go 90.47% <ø> (ø)
clientv3/watch.go 94.8% <ø> (ø)
etcdctl/ctlv3/command/make_mirror_command.go 15.11% <0%> (ø)
etcdctl/ctlv3/command/watch_command.go 15.15% <0%> (ø)
etcdctl/ctlv3/command/check.go 14.14% <0%> (ø)
clientv3/txn.go 100% <100%> (ø)
clientv3/auth.go 95.65% <100%> (ø)
clientv3/ready_wait.go 100% <100%> (ø)
clientv3/kv.go 96.77% <100%> (ø)
... and 12 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 ed92420...d44f7d5. Read the comment docs.

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.

3 participants