Tests for S3VersionStore::store_version_from_reader#404
Open
Tests for S3VersionStore::store_version_from_reader#404
Conversation
…producer task errors out--otherwise we may attempt to begin or continue uploads after sending abort_multipart_upload, which would be weird
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
malcolmgreaves
approved these changes
Mar 31, 2026
Collaborator
malcolmgreaves
left a comment
malcolmgreaves
left a comment
There was a problem hiding this comment.
Great PR w/ 1 requested change! Easy to follow. Great test cases and excellent in-line documentation. One requested change before merging -- please use workspace versions for hyper, hyper-util, s3s, and s3s-fs.
…ror that didn't originate from the aws sdk
…t multipart upload shenanigans
…3 without multipart upload shenanigans" because it fails at runtime without a size This reverts commit 7690d04.
…ge in the direct streaming attempt
…t; use file size to determine part size for multipart uploads for files > 100MB
CleanCut
force-pushed
the
cleancut/s3-tests
branch
from
e689414 to
16d3337
Compare
CleanCut
force-pushed
the
cleancut/s3-tests
branch
from
1ac0ab1 to
58091e1
Compare
Co-authored-by: Malcolm Greaves <malcolmgreaves@users.noreply.github.com>
…ires updating some dependencies
CleanCut
force-pushed
the
cleancut/s3-tests
branch
from
6cc41e0 to
f2c6737
Compare
CleanCut
added a commit
that referenced
this pull request
Apr 1, 2026
I'm tackling the S3 implementation one method at a time, and there's many more methods to go, so it will be a bit until we get into a state where we can actually try it out. But I have unit tests [in a separate PR](#404) (since they required some hefty dependency updates). - Implement the S3VersionStore::store_version_from_reader - Now requires specifying the file size up front, which all callers can easily do. - Uploads files <= 100MB in one shot (as per AWS recommendations) - Determines a file part size dynamically based on the file size for files > 100MB - Does not write to disk - Uploads up to 16 file parts concurrently - Cancels the multipart upload if anything goes wrong. - Add a new `OxenError::AwsS3Error` variant. - Updated the aws crates - Added instructions for claude to stop creating functions for random tiny bits of code just to call them exactly once and to stop deleting relevant comments🤞🏻 --------- Co-authored-by: Malcolm Greaves <malcolmgreaves@users.noreply.github.com>
Base automatically changed from
cleancut/implement-s3-store-version-from-reader
to
main
April 1, 2026 17:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: I won't blame you if you can't find this PR! 🤣 (Look at the PR number)
These tests are in a separate PR from #398 because pulling in
s3sas a dev dependency required a newerchrono, which required a newerduckdb, which required a newerarrow. It felt like isolating this in its own PR in case of side effects would be a good idea (and there was one observed side effect that had to be fixed).We need to get over this hurdle so we can have unit tests for the S3 implementation.