etcdctl: fix mk command with PrevNoExist #3685

gyuho · 2015-10-15T03:31:58Z

This attempts to fix #3676. PrevNoExist checks if the key previously exists and if so, it returns an error, which is how mk command is supposed to work. The previous code ignores the previous key and overwrites with the later value.

/cc @yichengq

gyuho · 2015-10-15T03:34:06Z

yichen says:

Create is just a wrapper around Set for simplicity, and you could use Set with PrevNoExist in etcdctl to make it fixed.

Will change it to use Set now.

yichengq · 2015-10-15T04:40:26Z

@gyuho Have you manually tested that it could resolve the bug in #3676 ?

Moreover, I would appreciate if you could write about problem and how it is fixed in the commit message, instead of writing too much context around API or previous PR.

gyuho · 2015-10-15T04:43:28Z

Yes, in my local machine, I ran the following command:

export INFRA_IP=127.0.0.1;

etcd -name infra0 \
    -initial-cluster-token my-infra \
    -initial-cluster infra0=http://$INFRA_IP:2380 \
    -initial-cluster-state new \
    -initial-advertise-peer-urls http://$INFRA_IP:2380 \
    -listen-peer-urls http://$INFRA_IP:2380 \
    -listen-client-urls http://$INFRA_IP:2379 \
    -advertise-client-urls http://$INFRA_IP:2379;

And use etcdctl:

$ etcdctl mk /foo hello
hello

$ etcdctl mk /foo hello
Error:  105: Key already exists (/foo) [8]

I changed the commit message to be more clearer.
Thanks,

Please let me know if we need more tests.

yichengq · 2015-10-15T14:45:12Z

etcdctl/command/mk_command.go

+	// By passing client.PrevNoExist the Set method checks if
+	// the key had existed previously, and returns error when
+	// it tries to overwrite the key, which is how mk command
+	// is supposed to work.


There is no need to mention PrevIgnore here because we don't use PrevIgnore here.

It would be better if you could make the second sentence more concrete.

gyuho · 2015-10-15T14:56:30Z

I agree. I made some changes on comments. Please let me know if you have any other feedback.

yichengq · 2015-10-15T15:02:22Z

etcdctl/command/mk_command.go

@@ -51,7 +51,9 @@ func mkCommandFunc(c *cli.Context, ki client.KeysAPI) {
 	ttl := c.Int("ttl")

 	ctx, cancel := contextWithTotalTimeout(c)
-	resp, err := ki.Set(ctx, key, value, &client.SetOptions{TTL: time.Duration(ttl) * time.Second, PrevExist: client.PrevIgnore})
+	// PrevNoExist means that the Node must not exist.
+	// Set method returns error if the key already exists.


Set method returns error if the key already exists. This sentence is not true if Set method doesn't use PrevNoExist.

Also the comments miss the part that why you explain how Set returns error here.

gyuho · 2015-10-15T15:05:49Z

@yichengq Sorry. I should have made it clear that the Set method in this case.

Is it clear now?

// Since PrevNoExist means that the Node must not exist,
// this Set method would return error if the key already exists.

yichengq · 2015-10-15T15:56:25Z

etcdctl/command/mk_command.go

@@ -51,7 +51,9 @@ func mkCommandFunc(c *cli.Context, ki client.KeysAPI) {
 	ttl := c.Int("ttl")

 	ctx, cancel := contextWithTotalTimeout(c)
-	resp, err := ki.Set(ctx, key, value, &client.SetOptions{TTL: time.Duration(ttl) * time.Second, PrevExist: client.PrevIgnore})
+	// Since PrevNoExist means that the Node must not exist,


must not exist previously

@yichengq

This attempts to fix etcd-io#3676. `PrevNoExist` checks if the key previously exists and if so, it returns an error, which is how `mk` command is supposed to work. The previous code ignores the previous key and overwrites with the later value. /cc @yichengq

gyuho · 2015-10-15T16:07:22Z

Since PrevNoExist means that the Node must not exist previously,
this Set method always creates a new key. Therefore, mk command
succeeds only if the key did not previously exist, and the command
prevents one from overwriting values accidentally.

This is the reason I think one would need use mk command for.
Please let me know if you have any other correction. Thanks!

yichengq · 2015-10-15T19:13:28Z

The comment is a little verbosal IMO, but it is good enough. 👍

LGTM. Thanks for fixing it!

etcdctl: fix mk command with PrevNoExist

gyuho · 2015-10-16T00:33:33Z

Thanks!

gyuho mentioned this pull request Oct 15, 2015

client, etcdctl: mk command with Create command #3679

Closed

gyuho force-pushed the etcdctl_mk_command_2 branch from 938abeb to 865c9c5 Compare October 15, 2015 03:47

gyuho changed the title ~~etcdctl/command: mk command with Create~~ etcdctl: use Set but with PrevNoExist for mk Oct 15, 2015

gyuho force-pushed the etcdctl_mk_command_2 branch from 865c9c5 to 7fe6bdb Compare October 15, 2015 04:49

gyuho changed the title ~~etcdctl: use Set but with PrevNoExist for mk~~ etcdctl: fix mk command with PrevNoExist Oct 15, 2015

yichengq reviewed Oct 15, 2015
View reviewed changes

gyuho force-pushed the etcdctl_mk_command_2 branch from 7fe6bdb to 0acc025 Compare October 15, 2015 14:55

gyuho force-pushed the etcdctl_mk_command_2 branch from 0acc025 to e0ac8e6 Compare October 15, 2015 14:57

yichengq reviewed Oct 15, 2015
View reviewed changes

gyuho force-pushed the etcdctl_mk_command_2 branch from e0ac8e6 to af8f4c7 Compare October 15, 2015 15:04

yichengq reviewed Oct 15, 2015
View reviewed changes

gyuho force-pushed the etcdctl_mk_command_2 branch from af8f4c7 to c3cf294 Compare October 15, 2015 16:04

gyuho force-pushed the etcdctl_mk_command_2 branch from c3cf294 to 1dab7e8 Compare October 15, 2015 16:05

yichengq added a commit that referenced this pull request Oct 15, 2015

Merge pull request #3685 from gyuho/etcdctl_mk_command_2

9ce79db

etcdctl: fix mk command with PrevNoExist

yichengq merged commit 9ce79db into etcd-io:master Oct 15, 2015

yichengq added the backport/v3.3 label Oct 15, 2015

gyuho deleted the etcdctl_mk_command_2 branch October 16, 2015 01:47

gyuho mentioned this pull request Dec 24, 2015

mk runs over existing keys (bug/documentation issue ?) #4068

Closed

gyuho removed the backport/v3.3 label Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcdctl: fix mk command with PrevNoExist #3685

etcdctl: fix mk command with PrevNoExist #3685

gyuho commented Oct 15, 2015

gyuho commented Oct 15, 2015

yichengq commented Oct 15, 2015

gyuho commented Oct 15, 2015

yichengq Oct 15, 2015

gyuho commented Oct 15, 2015

yichengq Oct 15, 2015

gyuho commented Oct 15, 2015

yichengq Oct 15, 2015

gyuho commented Oct 15, 2015

yichengq commented Oct 15, 2015

gyuho commented Oct 16, 2015

etcdctl: fix mk command with PrevNoExist #3685

etcdctl: fix mk command with PrevNoExist #3685

Conversation

gyuho commented Oct 15, 2015

gyuho commented Oct 15, 2015

yichengq commented Oct 15, 2015

gyuho commented Oct 15, 2015

yichengq Oct 15, 2015

Choose a reason for hiding this comment

gyuho commented Oct 15, 2015

yichengq Oct 15, 2015

Choose a reason for hiding this comment

gyuho commented Oct 15, 2015

yichengq Oct 15, 2015

Choose a reason for hiding this comment

gyuho commented Oct 15, 2015

yichengq commented Oct 15, 2015

gyuho commented Oct 16, 2015