Skip to content
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

Add a test for R2 multipart upload support to the test suite itself #273

Closed

Conversation

jakubadamw
Copy link
Contributor

I accidentally left it out in #260, so it doesn't actually run.

@jakubadamw
Copy link
Contributor Author

jakubadamw commented Jan 30, 2023

Unfortunately, miniflare does not support the multipart upload APIs yet, so the test doesn't work. A PR implementing it has just been opened, though!

@jakubadamw jakubadamw changed the title Add a missing test for R2 multipart upload support Actually run the test for R2 multipart upload support Jan 30, 2023
@jakubadamw jakubadamw changed the title Actually run the test for R2 multipart upload support Add a test for R2 multipart upload support to the test suite itself Jan 30, 2023
@zebp
Copy link
Collaborator

zebp commented Jan 31, 2023

That's fine, I gave this a manual test before merging and everything went smoothly when writing my own test. I did discover two issues with the existing test though, part numbers must be non-zero and parts must be the same size except for the last part.

The test should be changed to:

let chunk_sizes = [
    R2_MULTIPART_CHUNK_MIN_SIZE,
    R2_MULTIPART_CHUNK_MIN_SIZE,
    500,
];
let mut uploaded_parts = vec![];
for (chunk_index, chunk_size) in chunk_sizes.iter().copied().enumerate() {
    let chunk = vec![chunk_index as u8; chunk_size];
    uploaded_parts.push(upload.upload_part((chunk_index + 1) as u16, chunk).await?);
}
upload.complete(uploaded_parts).await?;

@jakubadamw
Copy link
Contributor Author

jakubadamw commented Feb 5, 2023

That's fine, I gave this a manual test before merging and everything went smoothly when writing my own test. I did discover two issues with the existing test though, part numbers must be non-zero and parts must be the same size except for the last part.

The test should be changed to:

let chunk_sizes = [
    R2_MULTIPART_CHUNK_MIN_SIZE,
    R2_MULTIPART_CHUNK_MIN_SIZE,
    500,
];
let mut uploaded_parts = vec![];
for (chunk_index, chunk_size) in chunk_sizes.iter().copied().enumerate() {
    let chunk = vec![chunk_index as u8; chunk_size];
    uploaded_parts.push(upload.upload_part((chunk_index + 1) as u16, chunk).await?);
}
upload.complete(uploaded_parts).await?;

@zebp thank you for reviewing that PR and for doing your own tests.

Are you sure the chunks (well, all but the last one) need to be precisely equal to R2_MULTIPART_CHUNK_MIN_SIZE? I think I have seen this work in a side project of mine where I would only make sure the chunks are at least that size, but not necessarily of equal size. In other words, I think that's simply the minimum size, but a chunk can be larger than that?

Agreed on the chunk index. I will update the test when the miniflare PR has been merged in.

@zebp
Copy link
Collaborator

zebp commented Feb 7, 2023

Are you sure the chunks (well, all but the last one) need to be precisely equal to R2_MULTIPART_CHUNK_MIN_SIZE?

They don't need to be the min length, but every chunk (except the last one) needs to be the same length. As long as the initial chunks are above 5M and the same size everything is fine. This has now been clarified in the docs for uploadPart.

@mrbbot
Copy link

mrbbot commented Feb 14, 2023

Hey! Miniflare 2.12.0 has just been released with support for multipart uploads, so this should unblock you. 🙂

@jakubadamw jakubadamw force-pushed the r2-multipart-uploads-test branch from b0a9d71 to be62636 Compare March 13, 2023 18:55
@jakubadamw jakubadamw closed this Apr 30, 2024
@jakubadamw jakubadamw deleted the r2-multipart-uploads-test branch April 30, 2024 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants