Make createMPU Call Async #84
Conversation
| stream: InputStream, | ||
| part_size: u64, | ||
| upload_id: String, |
There was a problem hiding this comment.
isn't upload id part of mpu_data?
Also the whole point of UploadContext was so to thread through all the state needed for an operation...
There was a problem hiding this comment.
isn't upload id part of mpu_data?
Thanks, fixed by not passing upload_id explicitly.
Also the whole point of UploadContext was so to thread through all the state needed for an operation...
Yeah, but the problem was that we no longer have mutable access to UploadContext in the createMPU task, because we do not own the handle at that point.
| @@ -252,8 +252,7 @@ async fn do_upload(args: Args) -> Result<(), BoxError> { | |||
| .bucket(bucket) | |||
| .key(key) | |||
| .body(stream) | |||
| .send() | |||
| .await?; | |||
| .initiate()?; | |||
There was a problem hiding this comment.
where'd you get the name initiate()? Is that a standard name used in similar libraries? Just want to be sure we pick something predictable for users
There was a problem hiding this comment.
Not really, Aaron suggested changing the name from send to avoid confusion with the Rust SDK at #73 (comment), and I couldn't think of something better than initiate/send.
| // TODO: We won't send completeMPU until customers join the future. This can create a | ||
| // bottleneck where we have many uploads not making the completeMPU call, waiting for the join | ||
| // to happen, and then everyone tries to do completeMPU at the same time. We should investigate doing | ||
| // this without waiting for join to happen. | ||
| complete_upload(self).await |
There was a problem hiding this comment.
I agree with the TODO. If small-file PutObject can complete before the user calls join(), then CompleteMPU should be sent before the user calls join.
whatever we do, we should try to be consistent for small and large objects,
| @@ -917,6 +917,6 @@ impl crate::operation::upload::input::UploadInputBuilder { | |||
| ) -> Result<UploadHandle, crate::error::Error> { | |||
| let mut fluent_builder = client.upload(); | |||
| fluent_builder.inner = self; | |||
| fluent_builder.send().await | |||
| fluent_builder.initiate() | |||
There was a problem hiding this comment.
if we're changing the name-and-asyncness of UploadFluentBuilder::send() then we should change the name-and-asyncness of UploadInputBuilder::send_with()
There was a problem hiding this comment.
Thanks, yeah, I can make it consistent once we have agreed on the initiate name in the previous feedback.
Description of changes:
This PR is just a refactor of the Upload APIs and doesn't make any functional changes.
upload_handle.abort()now takes ownership of the handle instead of a reference. It makes more sense to take ownership so that customers can't calljoinor do other things with the handle after aborting, as it won't be valid.join. From testing, completeMPU can be an expensive call for large objects.upload_idwas an Option because it won't be available for PutObject. I have moved it to theMultipartUploadDatastruct, and it will be non-optional wherever MPU is involved.test_cancel_single_upload_via_multipart_uploadtest to upload_tests.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.