Return errors instead of printing to logs #897
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #897 +/- ##
==========================================
+ Coverage 71.60% 73.26% +1.66%
==========================================
Files 11 11
Lines 1585 1575 -10
==========================================
+ Hits 1135 1154 +19
+ Misses 345 320 -25
+ Partials 105 101 -4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
With one exception, the logged errors are the result of calling a network connection Close() method on an error path. It's reasonable to ignore these close errors for two reasons:
- The functions are returning a more important error.
- There's nothing that the application can do with the information in the close error.
I would remove the calls to log.Printf by undoing the changes in 666c197.
If you are going to return the close error with errors.Join, then the secondary close error should come last in the list of errors. This does do things: the primary comes first in the error string; search on the error tree visits the primary error first.
The one exception is the error returned from bw.Flush(). That call should never return an error. It is correct to add the call to panic.
apoorvajagtap
force-pushed
the
return-errors
branch
from
b535127
to
b42aa3a
Compare
apoorvajagtap
force-pushed
the
return-errors
branch
from
b42aa3a
to
ff3115a
Compare
apoorvajagtap
force-pushed
the
return-errors
branch
from
471f923
to
c8a6785
Compare
|
This looks good to me. @GreenMarmot as you've been extremely helpful with these changes, do you see any further issues with this PR as it is? If not, I'd like to go ahead and merge it. |
What type of PR is this? (check all applicable)
Description
With #840 debug messages were printed to stderr, as this wasn't very application friendly. With this, the noise in the logs will be reduced, without impacting error handling.
Related Tickets & Documents
Added/updated tests?
Run verifications and test
make verifyis passingmake testis passing