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.
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
Add tests to check that requests are retried when writing/reading blobs on S3 #45383
Add tests to check that requests are retried when writing/reading blobs on S3 #45383
Changes from all commits
d306456
1d32f31
e58e779
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
we're relying on the fact that the implementation is ignoring the read limit, as the AWS SDK sets a low limit here (~131kb).
The mark/reset behavior of the SDK is also dependent on whether we have a
BufferedInputStream
(as you pointed out).It would be good to test the behavior of the actual inputstream that we are passing in.
In particular, the assertion added by #45224 is not sufficient to guarantee us retry behavior.
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.
As far as I understand the InputStream documentation, it seems that the read limit might or might not be respected by implementations. In any case the AWS SDK provides a default read limit (131kb) when marking the input stream but does not really expect the stream to respect the limit. The only exception is when the provided input stream is a
BufferedInputStream
and in this case the request client options read limit must be configured accordingly.I've been confused by this because at first I tried to use a
BufferedInputStream
in the test class but the AWS SDK has some special treatment for such streams, and since we don't provide buffered input stream to the client when snapshotting data I think it just does not make sense to use it here.I opened #45847 to beef up the tests a bit so that we sometimes upload blobs that are large than the default read limit and that sometimes we consumes part of the request body.
I agree, I'll add such tests as a follow up.