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

Simplify get_object by waiting for response headers #1171

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

passaro
Copy link
Contributor

@passaro passaro commented Nov 27, 2024

S3CrtClient::get_object was originally implemented so that it would complete immediately and return a GetObjectRequest implementation (extending Stream) to retrieve body parts. Any error from the S3 request would be returned through the stream.
We recently added additional methods (get_object_metadata in #1065 and get_object_checksum in #1123) to the response that rely on the headers returned by the (first) GetObject request. The new methods required an async signature and a complicated implementation in order to account for failures and they still do not correctly report accurate error information in some cases.
With this change, we modify get_object to await for response headers before returning either an error or a GetObjectResponse (note the name change) implementation. The ergonomics of get_object are improved:

  • awaiting the initial call can already return some errors (e.g. bucket/key not found),
  • get_object_checksum and get_object_metadata are now sync functions.

Does this change impact existing behavior?

Yes, get_object behavior is different, get_object_checksum and get_object_metadata signatures have changed, and GetObjectRequest was renamed to GetObjectResponse.

Does this change need a changelog entry?

Yes, it requires a breaking change entry for mountpoint-s3-client.


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).

@passaro passaro temporarily deployed to PR integration tests November 27, 2024 10:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 10:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 10:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 10:55 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 10:56 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 10:56 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 10:56 — with GitHub Actions Inactive
passaro added a commit to passaro/mountpoint-s3 that referenced this pull request Nov 27, 2024
Signed-off-by: Alessandro Passaro <[email protected]>
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 11:04 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 11:04 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 11:04 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 11:04 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 11:04 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 11:04 — with GitHub Actions Inactive
@passaro passaro marked this pull request as ready for review November 27, 2024 11:05
@passaro passaro temporarily deployed to PR integration tests November 27, 2024 11:28 — with GitHub Actions Inactive
headers = headers_receiver => headers.unwrap(),
result = request => {
// If we did not received the headers first, the request must have failed.
result?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This syntax is a little confusing on first read. Do you think something like result.err().or(...) would be more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I've gone back and forth with it. An alternative would be this:

return Err(result.err().unwrap_or_else(|| ObjectClientError::ClientError(S3RequestError::InternalError(Box::new(ObjectHeadersError::MissingHeaders)))));

Not sure which one is less readable...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to me now, but the crucial bit here, which requires some thinking to figure out, is that request will never be returned with S3GetObjectResponse if awaiting it already returned Poll::Ready.

Otherwise, awaiting on S3GetObjectResponse may block forever in some edge cases (given that S3HttpRequest::receiver is Fuse).

I'd add an assert!(!request.is_terminated()); before the Ok(..) return statement or protect from this possible bug in S3GetObjectResponse::poll_next or at least a comment.

passaro added a commit to passaro/mountpoint-s3 that referenced this pull request Nov 29, 2024
Signed-off-by: Alessandro Passaro <[email protected]>
@passaro passaro temporarily deployed to PR integration tests November 29, 2024 08:40 — with GitHub Actions Inactive
@passaro passaro temporarily deployed to PR integration tests November 29, 2024 08:40 — with GitHub Actions Inactive
Signed-off-by: Alessandro Passaro <[email protected]>
Signed-off-by: Alessandro Passaro <[email protected]>
Signed-off-by: Alessandro Passaro <[email protected]>
Signed-off-by: Alessandro Passaro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance PRs to run benchmarks on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants