Improve get_object interface for backpressure #1200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, we support flow-control window for GetObject requests by allowing applications to call `GetObjectResponse::increment_read_window` but it is tricky to use because we need to hold onto the stream itself in order to control the feedback loop while also consuming the data. This change introduces a new trait `ClientBackpressureHandle` for controlling the read window so that the stream and the flow-control paths are decoupled. Applications can now call `GetObjectResponse::take_backpressure_handle` to get a backpressure handle from the response and use this handle to extend the read window. Signed-off-by: Monthon Klongklaew <[email protected]>
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
Signed-off-by: Monthon Klongklaew <[email protected]>
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
reviewed
Dec 12, 2024
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
Signed-off-by: Monthon Klongklaew <[email protected]>
monthonk
force-pushed
the
improve_backpressure_get
branch
from
bd7c42e to
f472f60
Compare
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
reviewed
Dec 16, 2024
mountpoint-s3-client/CHANGELOG.md
Outdated
| @@ -8,6 +8,9 @@ | |||
| * `get_object` method now requires a `GetObjectParams` parameter. | |||
| Two of the existing parameters, `range` and `if_match` have been moved to `GetObjectParams`. | |||
| ([#1121](https://github.com/awslabs/mountpoint-s3/pull/1121)) | |||
| * `increment_read_window` and `read_window_end_offset` methods have been removed from `GetObjectResponse`. | |||
| `ClientBackpressureHandle` can be used to interact with flow-control window instead, it can be retrieved from `take_backpressure_handle` method. | |||
| headers: Headers, | ||
| /// Next offset of the data to be polled from [poll_next] | ||
| next_offset: u64, | ||
| /// Upper bound of the current read window. When backpressure is enabled, [S3GetObjectRequest] | ||
| /// can return data up to this offset *exclusively*. | ||
| read_window_end_offset: u64, | ||
| read_window_end_offset: Arc<AtomicU64>, |
There was a problem hiding this comment.
Do we need to keep a copy here? Couldn't we just use the one in backpressure_handle? I also suspect enable_backpressure may be redundant. Isn't it just backpressure_handle.is_some()?
There was a problem hiding this comment.
Yes, you're right. Updated for both CRT client and the mock client.
Signed-off-by: Monthon Klongklaew <[email protected]>
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
monthonk
temporarily deployed
to
PR integration tests
GitHub Actions
Inactive
passaro
approved these changes
Dec 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we support flow-control window for GetObject requests by allowing applications to call
GetObjectResponse::increment_read_windowbut it is tricky to use because we need to hold onto the stream itself in order to control the feedback loop while also consuming the data.This change introduces a new trait
ClientBackpressureHandlefor controlling the read window so that the stream and the flow-control paths are decoupled.Applications can now call
GetObjectResponse::take_backpressure_handleto get a backpressure handle from the response and use this handle to extend the read window.Does this change impact existing behavior?
Yes, there is a breaking change for
mountpoint-s3-client.Does this change need a changelog entry?
Yes, 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).