feat: [OCISDEV-1392] retry failed blob commit after postprocessing - #733
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| } | ||
| backoff *= 2 | ||
| } | ||
| if err = session.Finalize(ctx); err == nil { |
There was a problem hiding this comment.
will be better to log the error right after the function returns it but not in a L#309
Should we use log-level error instead of warning?
There was a problem hiding this comment.
Moved the log next to the error. Warn stays - retries are normal; the final failure logs Error in the caller.
|
@mklos-kw Should we have a backport to 8.2 stable? |
… large uploads don't get stuck in processing
cd78e9f to
c617c89
Compare
Good idea |
jvillafanez
left a comment
jvillafanez
left a comment
There was a problem hiding this comment.
Just minor details, nothing critical.
| if backoff > maxCommitRetryBackoff { | ||
| backoff = maxCommitRetryBackoff | ||
| } | ||
| log.Warn().Err(err).Int("attempt", attempt+1).Dur("backoff", backoff).Msg("blob commit failed, retrying after backoff") |
There was a problem hiding this comment.
Some minor details here:
- I'm not sure if the message will be a bit confusing. The attempt = 1 might refer to either the failed blob commit, but also to the retry attempt, so it might be confusing what you're counting there. Maybe "fails" or "failures" is clearer, or "retryNumber".
- We'll need some reference to the blob to be included in the log. If there are several failures from multiple uploads, knowing which message refers to which log entry will be annoying. I assume that the session has some information we can use to identify the uploads.
- The last attempt doesn't seem to be logged if it fails. Taking into account we have a maximum retry limit, I'd expect the same number of log messages if we reach the maximum retry count.
| case <-time.After(backoff): | ||
| } |
There was a problem hiding this comment.
I'd include a "do nothing" code comment just to ensure this is intentional and there isn't any missing code.
…alize path, add commit-failed metric
| spaceTypeIndex *spaceidindex.Index | ||
|
|
||
| // commitLimiter caps concurrent async blob commits at NumConsumers. | ||
| commitLimiter chan struct{} |
There was a problem hiding this comment.
Using a semaphore seems more standard. https://pkg.go.dev/golang.org/x/sync/semaphore
| if backoff > maxCommitRetryBackoff { | ||
| backoff = maxCommitRetryBackoff | ||
| } | ||
| backoff = min(backoff, maxCommitRetryBackoff) |
There was a problem hiding this comment.
Technically, the backoff could overflow, and the code could misbehave. I'd include a comment to say that is a risk we're taking and that the resulting behavior is acceptable.
Thank you, pushed fixes |
5 participants
retry failed blob commit after postprocessing so large uploads don't get stuck in processing