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

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

Closed
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

The "sledgehammer" of retrying.

WIP, untested.

Closes #xxxx
Copy link
Member

@bagder bagder left a comment

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?

/* 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");

This comment has been minimized.

@bagder

bagder Apr 4, 2020
Member

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

@@ -0,0 +1,19 @@
Long: retry-all-errors
Help: Retry all errors (use with --retry) (read manpage, don't use by default)

This comment has been minimized.

@bagder

bagder Apr 4, 2020
Member

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

@ryoqun
Copy link

@ryoqun 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 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 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
Copy link
Member

@bagder bagder left a comment

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

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.

This comment has been minimized.

@ryoqun

ryoqun Apr 16, 2020

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. :)

This comment has been minimized.

@jay

jay Apr 16, 2020
Author Member

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

@ryoqun
Copy link

@ryoqun 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 closed this in b995bb5 May 12, 2020
@jay jay deleted the jay:retry-all-errors branch May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.