Repository navigation
ref(preprod): Use regular PUTs for snapshot archive uploads - #128181
Conversation
Co-Authored-By: lcian <17258265+lcian@users.noreply.github.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8bbc763. Configure here.
jamieQ
left a comment
There was a problem hiding this comment.
Overall the changes lgtm. Nice that some of the logic will be handled automatically by the library! Left a couple minor comments.
| compress: Compression | None = None, | ||
| resumable_threshold_bytes: int | None = None, | ||
| ) -> str: | ||
| return self._primary.put( |
There was a problem hiding this comment.
IIUC the retry logic will change slightly after this change. Not sure how much it matters, but seems like 500s in the resumable upload path will now fail vs resulting in a retry attempt. This was noted by the bot review here, but unclear if that's an intentional decision or not.
There was a problem hiding this comment.
Leaving out 500s is intentional as you should get a 500 only when something goes really wrong internally.
Although the bot comment also mentions ConnectTimeoutError which we're not treating as retryable right now but I think we should, so I'm gonna make a patch release to do that, thanks for flagging.
Bump
objectstore-clientto 0.2.10 and upload snapshot ZIP archives with a simpleSession.putinstead of using the multipart API.Note that this means that other code paths where we do simple PUTs will also use resumables under the hood from now on, so this change affects more than preprod, most notably debug files which are the only likely files to exceed the default threshold of 256 MiB.
Motivation: the multipart API is going away and this was the only remaining usage of it in
sentry.Note that as of this version of
objectstore-client,Session.putbenefits from "automatic resumable uploads", meaning that calls toSession.putare translated to the new Resumable Uploads protocol internally when the provided stream is seekable and its length is above a certain threshold (which currently defaults to 256 MiB).The archive call sets
resumable_threshold_bytes=0to attempt resumable uploads for every archive (given that this is not a latency sensitive operation, we prefer to take the more reliable path always).Even though we remove multipart, this makes the large snapshot ZIP uploads resilient to failures, as with resumable uploads they would pick up the upload process where it was left instead of retrying the whole thing.
Multipart also potentially made the upload faster by uploading parts in parallel, but speed should generally not be an issue, as all transfers are happening inside the same VPC, so they should max out available bandwidth practically.
Close FS-474