-
Notifications
You must be signed in to change notification settings - Fork 157
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
Return the new object ETag in PutObjectResult #1057
Conversation
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
@@ -233,7 +226,7 @@ pub struct S3PutObjectRequest { | |||
start_time: Instant, | |||
total_bytes: u64, | |||
/// Headers of the CompleteMultipartUpload response, available after the request was finished | |||
response_headers: Arc<Mutex<Option<Headers>>>, | |||
response_headers: Receiver<Headers>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer response_headers_receiver
and update the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushing back a little on this. A Receiver
is a future and that's how it is used here. But happy to clarify in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just not clear from the naming that we have to call .await
to really get the headers, but not blocking on it.
Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! The failed test doesn't seem to relate to the change.
@@ -233,7 +226,7 @@ pub struct S3PutObjectRequest { | |||
start_time: Instant, | |||
total_bytes: u64, | |||
/// Headers of the CompleteMultipartUpload response, available after the request was finished | |||
response_headers: Arc<Mutex<Option<Headers>>>, | |||
response_headers: Receiver<Headers>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just not clear from the naming that we have to call .await
to really get the headers, but not blocking on it.
* Return the ETag in PutObjectResult Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk> * Simplify handling of response headers Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk> * Move ETag to a separate module Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk> * Add comments Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk> --------- Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
* Return the ETag in PutObjectResult Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk> * Simplify handling of response headers Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk> * Move ETag to a separate module Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk> * Add comments Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk> --------- Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>
Description of change
On success, both
put_object
(MPU) andput_object_single
return the ETag of the newly uploaded object.Does this change impact existing behavior?
No.
Does this change need a changelog entry in any of the crates?
Yes. Changelogs will be updated in a separate change (including changes from #1046).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).