-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
[WIP] tool: Add option --retry-all-errors to retry on any error #5185
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
Conversation
The "sledgehammer" of retrying. WIP, untested. Closes #xxxx
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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.
@@ -0,0 +1,19 @@ | |||
Long: retry-all-errors | |||
Help: Retry all errors (use with --retry) (read manpage, don't use by default) |
There was a problem hiding this comment.
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!
@jay Oh!! So cool! Thanks for working on this! And sorry for not getting my hands on this. I've been very busy lately.... |
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.
added
...
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. |
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. |
There was a problem hiding this 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!
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. |
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
For my use-case(#4656), this PR works nicely:
I'm very looking forward to the new release containing this. :) |
The "sledgehammer" of retrying.
WIP, untested.
Closes #xxxx
An alternative to --retry-flaky, which I abandoned. (#4558)
/cc @njsmith @ryoqun