Retry S3BlobContainer#getRegister on all exceptions#114813
Retry S3BlobContainer#getRegister on all exceptions#114813DaveCTurner merged 4 commits intoelastic:mainfrom
S3BlobContainer#getRegister on all exceptions#114813Conversation
S3 register reads are subject to the regular client retry policy, but in practice we see failures of these reads sometimes for errors that are transient but for which the SDK does not retry. This commit adds another layer of retries to these reads. Relates ES-9721
DaveCTurner
added
>enhancement
:Distributed/Snapshot/Restore
|
Documentation preview: |
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
| continue; | ||
| } catch (InterruptedException interruptedException) { | ||
| Thread.currentThread().interrupt(); | ||
| finalException.addSuppressed(interruptedException); |
There was a problem hiding this comment.
Should we break out of the loop if we're interrupted? I'm guessing something else in the loop will throw an InterruptedException if we continue, but would it be more polite to bail early?
There was a problem hiding this comment.
Yeah we do break out here, we fall through to the throw finalException line here. I added a comment.
| } else if (finalException == null) { | ||
| finalException = attemptException; | ||
| } else if (finalException != attemptException) { | ||
| finalException.addSuppressed(attemptException); |
There was a problem hiding this comment.
How is it possible that finalException == attemptException? does something we call throw a singleton exception? I'm not sure I follow this condition.
There was a problem hiding this comment.
Who knows what the SDK does or might do in future, but calling addSuppressed with the same exception throws an IllegalArgumentException and we don't want that here.
nicktindall
left a comment
nicktindall
left a comment
There was a problem hiding this comment.
LGTM with minor comments
S3 register reads are subject to the regular client retry policy, but in practice we see failures of these reads sometimes for errors that are transient but for which the SDK does not retry. This commit adds another layer of retries to these reads. Relates ES-9721
3 participants
S3 register reads are subject to the regular client retry policy, but in
practice we see failures of these reads sometimes for errors that are
transient but for which the SDK does not retry. This commit adds another
layer of retries to these reads.
Relates ES-9721