add warning message when delete to release-3.5 #13748

kkkkun · 2022-02-28T09:17:17Z

Base PR: #13729
Change commit branch from main to release-3.5

kkkkun · 2022-03-01T11:19:23Z

ptabor

If I understand correctly, the only case we want to issue a warning is case
that happens in line 72 of the original code:

opts = append(opts, clientv3.WithRange(args[1]))

i.e. the flags like --prefix', --from-keyor (to be added)--range` are not given...

The code should IMHO print warning only iff:

none of the flag is given
the command has 2 arguments.

BTW: I would still recommend submiting a PR to main adding the --range flag and warning .... and backport it to 3.5 later.

kkkkun · 2022-03-02T03:01:46Z

If I understand correctly, the only case we want to issue a warning is case that happens in line 72 of the original code:
opts = append(opts, clientv3.WithRange(args[1]))
i.e. the flags like --prefix', --from-keyor (to be added)--range` are not given...

The code should IMHO print warning only iff:

none of the flag is given

the command has 2 arguments.

BTW: I would still recommend submiting a PR to main adding the --range flag and warning .... and backport it to 3.5 later.

According to your advice. I sumbit this to release-3.5
and submit #13747 to main for 3.6 or later ?

ptabor

I think it does not work....
It prints warning only

etcdctl/ctlv3/command/del_command.go

ptabor · 2022-03-02T17:02:24Z

etcdctl/ctlv3/command/del_command.go

@@ -91,5 +96,9 @@ func getDelOp(args []string) (string, []clientv3.OpOption) {
 		opts = append(opts, clientv3.WithFromKey())
 	}

+	if !ignoreWarn {
+		fmt.Fprintf(os.Stderr, "Warning: will delete servarl keys.")


I would state it as:

Warning: This operations is implicitly deleting range of keys. Please add flag `--range` to make it explicit. Will sleep in v3.5.

I would state it as:

Warning: This operations is implicitly deleting range of keys. Please add flag `--range` to make it explicit. Will sleep in v3.5.

This PR commit to release-3.5?

Should we chang to this

Warning: This operations is implicitly deleting range of keys.

When commit to main:

> Warning: This operations is implicitly deleting range of keys. Please add flag `--range` to make it explicit. Will sleep in v3.6.

If we want add --range to 3.5. Please take a look #13747. If merged, we cherry-pick to release-3.5.

Do we need --range in release-3.5?

ahrtr · 2022-05-15T11:34:54Z

Why the warning message in this PR is different from the message in 13747 ?

This is a kind of enhancement, so in my opinion it should NOT be backported to 3.5. However it's just a very minor change, I will not insist on this if other maintainers/reviewers approve this PR.

kkkkun · 2022-05-17T02:32:32Z

Why the warning message in this PR is different from the message in 13747 ?

This is a kind of enhancement, so in my opinion it should NOT be backported to 3.5. However it's just a very minor change, I will not insist on this if other maintainers/reviewers approve this PR.

Copy from #13705 (comment)
It‘s just a warning message.

 In 3.5 we could add a warning on stderr.
 In 3.6 we could add support for --range and sleep (2s) if the --range is not given:
 a) let the operation be cancelled by human operator
 b) trigger attention of the script owners to start using the new flag.

etcdctl/ctlv3/command/del_command.go

ahrtr

LGTM

cc @serathius @spzala @ptabor Please see my comment issuecomment-1126916984.

ptabor

This PR adds Sleep(2sec) to 3.5, so has potential to break customer's workflows thus cannot be accepted.

I'm for submitting just warning+flag to 3.5, to let customer's prepare for 3.6, but the message needs to be adjusted.

kkkkun · 2022-06-13T09:31:29Z

This PR adds Sleep(2sec) to 3.5, so has potential to break customer's workflows thus cannot be accepted.

I'm for submitting just warning+flag to 3.5, to let customer's prepare for 3.6, but the message needs to be adjusted.

The message has been adjusted to #13748 (comment)

ahrtr · 2022-06-23T22:20:19Z

etcdctl/ctlv3/command/del_command.go

 	"go.etcd.io/etcd/pkg/v3/cobrautl"
+	"os"
+	"time"


Please try to group the import entries properly. The build-in package os, time and fmt should be grouped together and above other packages.

ahrtr · 2022-06-23T22:26:21Z

This PR adds Sleep(2sec) to 3.5, so has potential to break customer's workflows thus cannot be accepted.

I'm for submitting just warning+flag to 3.5, to let customer's prepare for 3.6, but the message needs to be adjusted.

@ptabor was requesting to remove the sleep, and I agree with that. Please also remove "Will sleep in v3.5." from the message as well, or change it to "Will sleep for a while in v3.6"?. cc @spzala for the professional opinion for the English syntax.

ahrtr · 2022-07-09T04:27:59Z

Please sign off the commit.

git commit --signoff --amend

ahrtr · 2022-07-10T04:35:11Z

Looks good to me now, @ptabor Do you have any comments?

ahrtr · 2022-10-09T06:14:48Z

Just to be clearer, this PR is a partially backporting of #13747

This is a minor enhancement for the etcdctl del command. Usually we only backport bug fix, so we shouldn't backport this PR. But since it's just a minor enhancement and it's useful, so I will not object if other maintainers agree to backport it.

What's your opinion? @ptabor @spzala @serathius If there is no response, then I assume there is no interest on this PR for 3.5. Then we can close it.

spzala · 2022-10-23T02:28:31Z

etcdctl/ctlv3/command/del_command.go

@@ -70,6 +73,9 @@ func getDelOp(args []string) (string, []clientv3.OpOption) {
 			cobrautl.ExitWithError(cobrautl.ExitBadArgs, fmt.Errorf("too many arguments, only accept one argument when `--prefix` or `--from-key` is set"))
 		}
 		opts = append(opts, clientv3.WithRange(args[1]))
+		if !delRange {
+			fmt.Fprintln(os.Stderr, "Warning: This operations is implicitly deleting range of keys. Please add flag `--range` to make it explicit. Will sleep for a while in v3.6.")
+		}


I agree with @ahrtr that Will sleep for a while... is difficult to interpret. Sorry, I took longer to review, but I would suggest something like these:
In etcd v3.6, along with this warning, the delete operation will be suspended for a few seconds to provide the user time to verify range.
or
In etcd v3.6, the operation will be suspended for a few seconds to provide the user time to verify range.
or
In v3.6, the operation will be suspended for a few seconds to provide the user time to verify range.
Thanks @kkkkun

ok, I would change to the second
In etcd v3.6, the operation will be suspended for a few seconds to provide the user time to verify range.

spzala · 2022-10-23T02:30:18Z

Just to be clearer, this PR is a partially backporting of #13747

This is a minor enhancement for the etcdctl del command. Usually we only backport bug fix, so we shouldn't backport this PR. But since it's just a minor enhancement and it's useful, so I will not object if other maintainers agree to backport it.

What's your opinion? @ptabor @spzala @serathius If there is no response, then I assume there is no interest on this PR for 3.5. Then we can close it.

@ahrtr I agree with your thoughts here. I am okay with backporting. Thanks!

Signed-off-by: kkkkun <scuzk373x@gmail.com>

ahrtr · 2022-11-05T08:50:56Z

Thanks @kkkkun

ptabor suggested changes Mar 1, 2022

View reviewed changes

kkkkun mentioned this pull request Mar 2, 2022

add flag range for del in etcdctl #13747

Merged

kkkkun force-pushed the add-warning-for-del branch 2 times, most recently from 1960aa6 to 6801502 Compare March 2, 2022 06:52

kkkkun changed the title ~~add warning message when delete~~ add warning message when delete to release-3.5 Mar 2, 2022

ptabor suggested changes Mar 2, 2022

View reviewed changes

kkkkun force-pushed the add-warning-for-del branch from 6801502 to 445836b Compare March 6, 2022 06:07

kkkkun requested a review from ptabor March 6, 2022 06:07

serathius mentioned this pull request Apr 6, 2022

Plans for v3.5.3 release #13894

Closed

28 tasks

kkkkun force-pushed the add-warning-for-del branch from 445836b to 3fb7a93 Compare May 17, 2022 02:28

ahrtr reviewed May 17, 2022

View reviewed changes

etcdctl/ctlv3/command/del_command.go Show resolved Hide resolved

ahrtr reviewed Jun 13, 2022

View reviewed changes

etcdctl/ctlv3/command/del_command.go Outdated Show resolved Hide resolved

kkkkun force-pushed the add-warning-for-del branch from 3fb7a93 to 6b9e981 Compare June 13, 2022 03:34

ahrtr approved these changes Jun 13, 2022

View reviewed changes

ptabor suggested changes Jun 13, 2022

View reviewed changes

kkkkun force-pushed the add-warning-for-del branch from 6b9e981 to f055a4f Compare June 13, 2022 09:29

kkkkun force-pushed the add-warning-for-del branch 2 times, most recently from 5a59b27 to 25ff3c7 Compare June 14, 2022 03:33

ahrtr reviewed Jun 23, 2022

View reviewed changes

kkkkun force-pushed the add-warning-for-del branch from 25ff3c7 to c529c47 Compare July 9, 2022 03:53

kkkkun force-pushed the add-warning-for-del branch 2 times, most recently from 4c2715d to 00f1987 Compare July 10, 2022 02:29

spzala reviewed Oct 23, 2022

View reviewed changes

add range flag for delete in etcdctl

6797856

Signed-off-by: kkkkun <scuzk373x@gmail.com>

kkkkun force-pushed the add-warning-for-del branch from 00f1987 to 6797856 Compare November 5, 2022 06:34

ahrtr merged commit c2378be into etcd-io:release-3.5 Nov 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add warning message when delete to release-3.5 #13748

add warning message when delete to release-3.5 #13748

kkkkun commented Feb 28, 2022 •

edited

Loading

kkkkun commented Mar 1, 2022

ptabor left a comment

kkkkun commented Mar 2, 2022 •

edited

Loading

ptabor left a comment

ptabor Mar 2, 2022

kkkkun Mar 3, 2022 •

edited

Loading

ahrtr commented May 15, 2022

kkkkun commented May 17, 2022

ahrtr left a comment

ptabor left a comment

kkkkun commented Jun 13, 2022 •

edited

Loading

ahrtr Jun 23, 2022

ahrtr commented Jun 23, 2022

ahrtr commented Jul 9, 2022

ahrtr commented Jul 10, 2022

ahrtr commented Oct 9, 2022 •

edited

Loading

spzala Oct 23, 2022

kkkkun Oct 25, 2022

kkkkun Nov 5, 2022

spzala commented Oct 23, 2022

ahrtr commented Nov 5, 2022

add warning message when delete to release-3.5 #13748

add warning message when delete to release-3.5 #13748

Conversation

kkkkun commented Feb 28, 2022 • edited Loading

kkkkun commented Mar 1, 2022

ptabor left a comment

Choose a reason for hiding this comment

kkkkun commented Mar 2, 2022 • edited Loading

ptabor left a comment

Choose a reason for hiding this comment

ptabor Mar 2, 2022

Choose a reason for hiding this comment

kkkkun Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

ahrtr commented May 15, 2022

kkkkun commented May 17, 2022

ahrtr left a comment

Choose a reason for hiding this comment

ptabor left a comment

Choose a reason for hiding this comment

kkkkun commented Jun 13, 2022 • edited Loading

ahrtr Jun 23, 2022

Choose a reason for hiding this comment

ahrtr commented Jun 23, 2022

ahrtr commented Jul 9, 2022

ahrtr commented Jul 10, 2022

ahrtr commented Oct 9, 2022 • edited Loading

spzala Oct 23, 2022

Choose a reason for hiding this comment

kkkkun Oct 25, 2022

Choose a reason for hiding this comment

kkkkun Nov 5, 2022

Choose a reason for hiding this comment

spzala commented Oct 23, 2022

ahrtr commented Nov 5, 2022

kkkkun commented Feb 28, 2022 •

edited

Loading

kkkkun commented Mar 2, 2022 •

edited

Loading

kkkkun Mar 3, 2022 •

edited

Loading

kkkkun commented Jun 13, 2022 •

edited

Loading

ahrtr commented Oct 9, 2022 •

edited

Loading