-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
logcli: Add retries to unsuccessful log queries #3879
Conversation
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.
Thanks for adding this feature
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.
Just added a minor suggestion but otherwise the code LGTM!
Thanks for the PR!
pkg/logcli/client/client.go
Outdated
retries := c.Retries | ||
success := false | ||
|
||
for retries > 0 { |
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.
If a user sets the retries flag value to 0 by mistake then they would unexpectedly skip making any calls. Also, retried calls should be the ones done after seeing a failure in the initial call, so let us initialise the retries
above to c.Retries + 1
.
What do you think?
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.
You are right, but I enhanced your idea a little bit. I renamed the variable to "attempts", which seemed like a better name to me now. I implemented your suggestion, so attempts = c.Retries + 1
and I made the default value for c.Retries = 0. So there still is 1 attempt unless the user sets the retries flag to something other than 0.
Number of attempts for each query = retries + 1 Default number of retries = 0 So by default, there is always 1 attempt for each query
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.
LGTM!
What this PR does / why we need it:
This adds a possibility to retry unsuccessful queries when using logcli
Example output of a failed query (there is no server running on that port):
Which issue(s) this PR fixes:
Fixes #3855
Special notes for your reviewer:
Checklist