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
Don't ignore errors in UI API Layer #2581
Don't ignore errors in UI API Layer #2581
Conversation
fmt.Fprintln(w, data) | ||
_, err := fmt.Fprintln(w, data) | ||
if err != nil { | ||
glog.Warningf("Unable to write response body. Cause: %v", err) |
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.
log as an error
body.Close() | ||
err := iosafety.DrainReader(body) | ||
if err != nil { | ||
glog.Warningf("Unable to drain body reader. Cause: %v", err) |
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.
maybe it would be good to return these errors. If this is not an option, log them with error level
flag.Set("stderrthreshold", "INFO") | ||
err := flag.Set("stderrthreshold", "INFO") | ||
if err != nil { | ||
glog.Error(errors.Wrap(err, "while parsing flags")) |
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.
Please consider glog.Fatal
- but it's totally up to you
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 decided that the error occurring during setting that flag wasn't critical because it wasn't really related to parsing user provided flags and that logging the error without exiting the application would suffice.
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.
Great work.
Description
Changes proposed in this pull request:
Related issue(s)
#2113 #2587