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

Return the new object ETag in PutObjectResult #1057

Merged
merged 4 commits into from
Oct 15, 2024

Conversation

passaro
Copy link
Contributor

@passaro passaro commented Oct 14, 2024

Description of change

On success, both put_object (MPU) and put_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).

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>,
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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>
@passaro passaro temporarily deployed to PR integration tests October 15, 2024 11:10 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests October 15, 2024 11:10 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests October 15, 2024 11:11 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests October 15, 2024 11:11 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests October 15, 2024 11:11 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests October 15, 2024 11:11 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests October 15, 2024 11:11 — with GitHub Actions Inactive
Copy link
Contributor

@monthonk monthonk left a 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>,
Copy link
Contributor

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.

@passaro passaro added this pull request to the merge queue Oct 15, 2024
Merged via the queue into awslabs:main with commit e98a5c2 Oct 15, 2024
23 checks passed
@passaro passaro deleted the put-object-result branch October 15, 2024 16:08
rajdchak pushed a commit to rajdchak/mountpoint-s3-fork that referenced this pull request Oct 16, 2024
* 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>
rajdchak pushed a commit to rajdchak/mountpoint-s3-fork that referenced this pull request Oct 16, 2024
* 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>
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.

2 participants