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
Retry prepareUploadParts
on fail for @uppy/aws-s3-multipart
#3224
Conversation
@martin-brennan if you have the chance to review this before your holiday that would be great :) |
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.
Code LGTM, shouldn't we use async/await for the tests?
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.
Thanks for doing this! I think the code looks fine and the other commenters have things like async
changes covered. I think the retrying only occurring under certain conditions, along with the implementer needing to return Promise.reject({ source: { status: 500 } })
is fine as long as it is documented. Maybe there could be a section on retrying here:
uppy/website/src/docs/aws-s3-multipart.md
Lines 47 to 52 in 2a1ac74
### `retryDelays: [0, 1000, 3000, 5000]` | |
When uploading a chunk to S3 using a presigned URL fails, automatically try again after the millisecond intervals specified in this array. By default, we first retry instantly; if that fails, we retry after 1 second; if that fails, we retry after 3 seconds, etc. | |
Set to `null` to disable automatic retries, and fail instantly if any chunk fails to upload. |
So you could talk about which functions are retried (uploadPart and prepareUploadParts), the retry delays, and the conditions which cause a retry.
At some point it may make sense to make the retry conditions a configurable function? E.g.
just allow shouldRetry
to be an option like prepareUploadParts
:
uppy/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js
Lines 216 to 223 in 2a1ac74
function shouldRetry (err) { | |
if (err.source && typeof err.source.status === 'number') { | |
const { status } = err.source | |
// 0 probably indicates network failure | |
return status === 0 || status === 409 || status === 423 || (status >= 500 && status < 600) | |
} | |
return false | |
} |
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 that you fixed the inconsistencies in the docs 👍
IMHO it's less cluttered and easier to read colored js
code instead of more cluttered and unicolor json
markdown. So I suggested changing it to js instead, if you agree.
{ | ||
"presignedUrls": { | ||
"1": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=1&...", | ||
"2": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=2&...", | ||
"3": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=3&..." | ||
}, | ||
"headers": { "some-header": "value" } | ||
} |
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.
{ | |
"presignedUrls": { | |
"1": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=1&...", | |
"2": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=2&...", | |
"3": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=3&..." | |
}, | |
"headers": { "some-header": "value" } | |
} | |
{ | |
presignedUrls: { | |
1: 'https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=1&...', | |
2: 'https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=2&...', | |
3: 'https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=3&...', | |
}, | |
headers: { 'some-header': 'value' }, | |
} |
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 don't think the linter is going to like that. FWIW I think JSON is fine here.
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.
Oh I didn't know that we also lint js blocks inside markdown.
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'm also inclined to keep it the way it is.
Co-authored-by: Mikael Finstad <finstaden@gmail.com>
Fixes #3190
Questions
The current retry implementation only retries under these conditions:
uppy/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js
Lines 216 to 223 in a298e59
This means the implementer must return something like this when their custom
prepareUploadParts
fails:Do we want to have the same logic for retrying
prepareUploadParts
? Something else? Either way it would need to be documented.