-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Retry on interrupted connections when reading from S3 in native FS library #22895
Conversation
The solution is based on aws/aws-sdk-java#856 (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.
LGTM!
lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Retries.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Retries.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Retries.java
Show resolved
Hide resolved
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.
Testing this kind of stuff is hard. I like the usage of Toxiproxy. It's very cool that we can wrap this around an arbitrary service. The test verifies that retries are happening, which is good enough, so the below is just a thought/suggestion.
It would be nice to test that reads succeed after the retry. Given that S3 is simple and the request/response pattern is also simple, using MockWebServer
might be a good alternative. You could enqueue multiple failure responses for the failure case, and failure+success responses for the success-with-retry case.
lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3Input.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3Input.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Retries.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Retries.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Retries.java
Outdated
Show resolved
Hide resolved
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.
🎉
lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Retries.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Retries.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Retries.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3Input.java
Outdated
Show resolved
Hide resolved
lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3Input.java
Outdated
Show resolved
Hide resolved
Read the resposne inside a transformer lambda and throw a retryable exception on IO errors to use the already configured SDK retry mechanism.
178de2a
to
a54960e
Compare
@electrum I first tried doing this with the Maybe that can be a follow-up, I applied all comments and this should be ready to go. |
Description
Read the response inside a transformer lambda and throw a retryable
exception on IO errors to use the already configured SDK retry mechanism.
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: