-
Notifications
You must be signed in to change notification settings - Fork 274
fix(amazonq): retry S3 upload #5208
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
Conversation
| Thread.sleep((1000 * 2.0.pow(i)).toLong()) | ||
| } | ||
| } | ||
| LOG.info { "Upload to S3 succeeded" } |
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 not a big fan of implementing our own retry logic rather than doing it at the SDK layer, but the existing S3 client's upload functionality requires a bucket name as a part of the request, and 1) I don't think we should have the internal S3 bucket names we use floating around this repo, 2) we use multiple different buckets, and 3) we don't have access to the bucket name in the response of our CreateUploadUrl — this API would have to be changed and then we can use the existing S3 client.
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.
Update: use waitUntil instead of a for loop; makes this retry logic even simpler
| UnknownHostException::class, | ||
| SocketTimeoutException::class, | ||
| HttpRequests.HttpStatusException::class, | ||
| ConnectException::class |
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.
UnknownHostException and SocketTimeoutException can be reproduced by turning off your WiFi, HttpStatusException could be a 500 or something, and I've occasionally seen ConnectException too, so I think all of these are worth retrying on.
.changes/next-release/bugfix-552a4676-3214-4321-a332-344bdf9f1830.json
Outdated
Show resolved
Hide resolved
…830.json Co-authored-by: Richard Li <[email protected]>
* fix(amazonq): retry S3 upload * fix test * fix detekt * use waitUntil * update changelog * fix detekt * fix detekt again * remove unused import * fix tests * Update .changes/next-release/bugfix-552a4676-3214-4321-a332-344bdf9f1830.json Co-authored-by: Richard Li <[email protected]> --------- Co-authored-by: David Hasani <[email protected]> Co-authored-by: Richard Li <[email protected]>
* fix(amazonq): retry S3 upload * fix test * fix detekt * use waitUntil * update changelog * fix detekt * fix detekt again * remove unused import * fix tests * Update .changes/next-release/bugfix-552a4676-3214-4321-a332-344bdf9f1830.json Co-authored-by: Richard Li <[email protected]> --------- Co-authored-by: David Hasani <[email protected]> Co-authored-by: Richard Li <[email protected]>
Types of changes
Description
Retry S3 upload.
Checklist
License
I confirm that my contribution is made under the terms of the Apache 2.0 license.