Fix log for archival #5819
Fix log for archival #5819
Conversation
davidporter-id-au
requested review from
Shaddoll,
neil-xie,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Pull Request Test Coverage Report for Build 018e809c-3d58-41a7-b684-2d08f136e522Details
💛 - Coveralls |
Codecov Report
Additional details and impacted files
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -89,11 +89,17 @@ func uploadHistoryActivity(ctx context.Context, request ArchiveRequest) (err err | |||
| if err == nil { | |||
| return nil | |||
| } | |||
| if err.Error() == errUploadNonRetriable.Error() { | |||
| logger.Error(carchiver.ArchiveNonRetriableErrorMsg, tag.ArchivalArchiveFailReason("got non-retryable error from history archiver")) | |||
| if errors.Is(err, errUploadNonRetriable) { | |||
There was a problem hiding this comment.
Superfically seems fine, but to double check: are we returning that exact error value?
Comparing strings is a looser check in many common ways, so generally this would be less likely to be true than it was before. Since I don't think we have wrapped errors here. (Or do we? This looks isolated enough that it seems possible anyway)
There was a problem hiding this comment.
I don't know what error we're returning because the log isn't showing anything useful, it's not got the error tag
There was a problem hiding this comment.
I did check this is coverd by a unit test before changing the errors.Is, I could remove it if it's a concern, but I don't really see a lot of values in jsut string comparing like this
What changed?
The log to work out what's nonretriable is not logged
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes