scaffold download objects operation #39
Conversation
| @@ -51,6 +51,10 @@ pub struct Args { | |||
| /// Enable tokio console (requires RUSTFLAGS="--cfg tokio_unstable") | |||
| #[arg(long, default_value_t = false, action = clap::ArgAction::SetTrue)] | |||
| tokio_console: bool, | |||
|
|
|||
| /// Command is performed on all files or objects under the specified directory or prefix | |||
There was a problem hiding this comment.
It's an S3 concept. Not sure if you're just rhetorically asking for more docs here
| /// The number of objects that failed to be downloaded | ||
| pub objects_failed: u64, | ||
|
|
||
| /// A list of failed object transfers | ||
| pub failed_transfers: Option<Vec<FailedDownloadTransfer>>, |
There was a problem hiding this comment.
trivial / educate-me: why not just have a Vec? why have both a Vec AND another field which is the length of that Vec? Is there a possibility that objects_failed != failed_transfers.unwrap().len()?
also, why is the Vec wrapped in an Option? Is that common? Why not just have a Vec whose length may be 0?
There was a problem hiding this comment.
Good question, SEP said to have the fields in the response:
Response
| Field | Description | Type | Required? | Default Value |
|---|---|---|---|---|
| objectsUploaded | The number of objects that have been uploaded | long | Required | N/A |
| objectsFailed | The number of objects that have failed | long | Required | N/A |
But then later it says to enumerate failures. I added the Vec after originally defining it and tend to agree it's redundant. I'll remove it for now unless anyone feels otherwise.
| #[derive(Debug)] | ||
| pub struct DownloadObjectsOutput { | ||
| /// The number of objects that were successfully downloaded | ||
| pub objects_downloaded: u64, |
There was a problem hiding this comment.
don't need this now, but would we also want to offer a list of successful transfers?
There was a problem hiding this comment.
It could be very large, I would think we'd want that to be opt-in or something.
aws-s3-transfer-manager/src/operation/download_objects/builders.rs
Outdated
Show resolved
Hide resolved
| crate::operation::download_objects::DownloadObjects::orchestrate(self.handle, input).await | ||
| } | ||
|
|
||
| /// Set the bucket name containing the object(s) to download. |
There was a problem hiding this comment.
Writing a macro can help reduce the boilerplate for XXX and set_XXX, eventually.
There was a problem hiding this comment.
Yeah I thought about that, we only have one more operation to go but we'll see.
aws-s3-transfer-manager/src/types.rs
Outdated
| /// A filter for downloading objects from S3 | ||
| #[derive(Clone)] | ||
| pub struct DownloadFilter { | ||
| pub(crate) predicate: Arc<dyn Fn(&aws_sdk_s3::types::Object) -> bool>, |
There was a problem hiding this comment.
May also need Send + Sync bound at the end, i.e.
Arc<dyn Fn(&aws_sdk_s3::types::Object) -> bool + Send + Sync>,
but no need to change it. The compiler can tell us if we do need it.
There was a problem hiding this comment.
Yeah agreed. Also I don't know if i like this representation. I almost wonder if the closure shouldn't just be stored directly on the input type but I opted for defining a new type DownloadFilter for it should we find that it makes sense to change or expand it in some way.
Issue #, if available:
#8
Description of changes:
This PR adds just the scaffolding for a new
download_objectsoperation which will download one or more objects from a bucket and write the results to a directory. This is the first operation with a bit more liberty in the operation input/output type representations so pay attention to those and what the SEP asks for and whether you agree with what's proposed.Questions/Discuss
Type for
DownloadFilter?SEP calls out having
getObjectRequestCallbackwhich is described asHave to think through how we want to do this
Java v2 TM also has a listObjectsV2Transformer for transforming the list objects request. This isn't called out in the SEP, unsure if we want to or should consider equivalent functionality.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.