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.

Sign up for GitHub

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

Jump to bottom

[WIP] tool: Add option --retry-all-errors to retry on any error #5185

Closed
wants to merge 3 commits into from
Closed

[WIP] tool: Add option --retry-all-errors to retry on any error #5185

wants to merge 3 commits into from

Conversation

jay
Copy link
Member

@jay jay commented Apr 4, 2020

The "sledgehammer" of retrying.

WIP, untested.

Closes #xxxx

An alternative to --retry-flaky, which I abandoned. (#4558)

/cc @njsmith @ryoqun

@jay
tool: Add option --retry-all-errors to retry on any error
852176d 
The "sledgehammer" of retrying.

WIP, untested.

Closes #xxxx
@jay jay mentioned this pull request Apr 4, 2020
Closed
bagder
bagder reviewed Apr 4, 2020
View reviewed changes
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Do you think maybe this option should rather be made so that it can be used instead of --retry ? I can't think of any obvious benefit from telling users to use both.

Have you tried to write up a test case for this as well?

src/tool_operate.c Outdated
/* Warn the first time --retry-all-errors is used for the transfer. */
if(!per->config->retry_all_errors_warned) {
warnf(global,
"Option --retry-all-errors may have unintended consequences.\n");
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I think outputting a warning here just for the option being used is a good idea.

docs/cmdline-opts/retry-all-errors.d
@@ -0,0 +1,19 @@
Long: retry-all-errors
Help: Retry all errors (use with --retry) (read manpage, don't use by default)
Copy link
Member

Choose a reason for hiding this comment

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

I think you should remove the second parenthesis. Users need to read the man page to understand all options!

@ryoqun
Copy link

ryoqun commented Apr 5, 2020

@jay Oh!! So cool! Thanks for working on this! And sorry for not getting my hands on this. I've been very busy lately....

@jay
Copy link
Member Author

jay commented Apr 6, 2020

Do you think maybe this option should rather be made so that it can be used instead of --retry ? I can't think of any obvious benefit from telling users to use both.

I think it should rely on --retry since we have another --retry that does that and I like the consistency. Otherwise we'd need a way to specify the number of retries so instead of --retry 5 --retry-all-errors it would be --retry-all-errors 5 which I think is harder to understand.

Have you tried to write up a test case for this as well?

added

I'm not sure I think outputting a warning here just for the option being used is a good idea.

...

I think you should remove the second parenthesis. Users need to read the man page to understand all options!

The warning only shows once per transfer if a retry occurs due to the option. I think there will be a greater risk of user error with the option.

@bagder
Copy link
Member

bagder commented Apr 6, 2020

The warning only shows once per transfer if a retry occurs due to the option

Still, that's warning shown to users even when doing everything correctly and there's not even a hint of them having misunderstood or misused the option. I don't like that.

bagder
bagder approved these changes Apr 9, 2020
View reviewed changes
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Should be good to merge as soon as the feature Window reopens!

@jay jay added the feature-window A merge of this requires an open feature window label Apr 12, 2020
ryoqun
ryoqun reviewed Apr 16, 2020
View reviewed changes
docs/cmdline-opts/retry-all-errors.d
Help: Retry all errors (use with --retry) (read manpage, don't use by default)
Added: 7.71.0
---
Retry on any error. This option is used together with --retry.
Copy link

Choose a reason for hiding this comment

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

How about mentioning --retry-all-errors in the description of --retry to mention the transient errors can be extended to all kinds of errors by --retry-all-errors? This will make users notice this new shining option more easily. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not it's a more advanced option.

@ryoqun
Copy link

ryoqun commented Apr 16, 2020

For my use-case(#4656), this PR works nicely:

$ sleep 6 && echo hello > something-is-ready & ./src/curl -v --retry 10 --retry-delay 2 --retry-all-errors file://$(realpath something-is-ready) && echo $?
[1] 30289
* Couldn't open file /home/ryoqun/work/curl/curl/something-is-ready
* Closing connection -1
curl: (37) Couldn't open file /home/ryoqun/work/curl/curl/something-is-ready
Warning: Transient problem: (retrying all errors) Will retry in 2 seconds. 10
Warning: retries left.
* Couldn't open file /home/ryoqun/work/curl/curl/something-is-ready
* Closing connection -1
curl: (37) Couldn't open file /home/ryoqun/work/curl/curl/something-is-ready
Warning: Transient problem: (retrying all errors) Will retry in 2 seconds. 9
Warning: retries left.
* Couldn't open file /home/ryoqun/work/curl/curl/something-is-ready
* Closing connection -1
curl: (37) Couldn't open file /home/ryoqun/work/curl/curl/something-is-ready
Warning: Transient problem: (retrying all errors) Will retry in 2 seconds. 8
Warning: retries left.
hello
* Closing connection 0
[1]+  Done                    sleep 6 && echo hello > something-is-ready
0

I'm very looking forward to the new release containing this. :)

@jay jay mentioned this pull request May 2, 2020
Closed
@jay jay closed this in b995bb5 May 12, 2020
@jay jay deleted the retry-all-errors branch May 12, 2020 07:01
@jay jay removed the feature-window A merge of this requires an open feature window label May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryoqun ryoqun ryoqun left review comments

@bagder bagder bagder approved these changes

Assignees
No one assigned
Labels
cmdline tool
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jay @ryoqun @bagder