s3 example - thumbnail creator (#613) #621
Conversation
peterborkuti
force-pushed
the
s3-example-thumbnail
branch
from
b6d0ed6 to
07cf14f
Compare
| #[async_trait] | ||
| impl GetFile for S3Client { | ||
| async fn get_file(&self, bucket: &str, key: &str) -> Option<Cursor<Vec<u8>>> { | ||
| println!("get file bucket {}, key {}", bucket, key); |
There was a problem hiding this comment.
np: what do you think about making these tracing::info calls?
|
|
||
| #[async_trait] | ||
| pub trait GetFile { | ||
| async fn get_file(&self, bucket: &str, key: &str) -> Option<Cursor<Vec<u8>>>; |
There was a problem hiding this comment.
I'd recommend this trait also return a Result and let the upper layer handle the error as it sees fit
|
Thank you for the thorough review. Give me some days to fix your findings. |
|
Dear @greenwoodcm and @timClicks, |
|
I checked the performance, so I uploaded the same image with different names:
It created different events, so every upload initiated one handler run. I moved the client instantiation into the handler and checked the same way, the duration was between 122ms and 185ms.
|
peterborkuti
force-pushed
the
s3-example-thumbnail
branch
from
b1ccc2f to
6984cdd
Compare
| } | ||
|
|
||
| fn get_file_props(record: &S3EventRecord) -> (String, String) { | ||
| let empty_response = ("".to_string(), "".to_string()); |
There was a problem hiding this comment.
instead of using the empty string to represent nothing here, i recommend having this function return (Option<String>, Option<String>)
| Ok(()) | ||
| } | ||
|
|
||
| fn get_file_props(record: &S3EventRecord) -> (String, String) { |
There was a problem hiding this comment.
it looks like you'd be able to create your iterator using .into_iter() and then pass an owned S3EventRecord into this method. then you don't have to .to_owned() below, which i believe implicitly clones.
| } | ||
|
|
||
| let bucket_name = record.s3.bucket.name.to_owned().unwrap(); | ||
| let object_key = record.s3.object.key.to_owned().unwrap(); |
There was a problem hiding this comment.
instead of doing the if checks above then unwrapping here, i recommend a match statement.
match (record.event_name, record.s3.bucket.name) {
(Some(event_name), Some(bucket_name) => {
// additional validation
}
_other => (None, None),
}
this can even be used as the return statement, without using the return keyword
| let reader = Cursor::new(vec); | ||
| let mut thumbnails = create_thumbnails(reader, mime::IMAGE_PNG, [ThumbnailSize::Custom((size, size))]).unwrap(); | ||
|
|
||
| let thumbnail = thumbnails.pop().unwrap(); |
There was a problem hiding this comment.
you might want to propagate errors up through this method so that if the method fails (for whatever reason) it doesn't crash the function
|
Hi @greenwoodcm, |
Issue #613, if available:
Description of changes:
S3 example: donwload + create thumbnail + upload
It uses a shared client, based on the example #619
By submitting this pull request