API, AWS: Retry S3InputStream reads #10433

amogh-jahagirdar · 2024-06-03T19:03:09Z

This is an alternative approach to https://github.com/apache/iceberg/pull/4912/files and https://github.com/apache/iceberg/pull/8221/files#diff-0b632866a3b10fac55c442b08178ec0ac72b3b600878243e15d788a8bd031054

for retrying failures encountered when retrying on the reading of input streams.

This approach defines a RetryableInputStream class which will wrap underlying input streams returned by object store APIs.
Upon failures a new stream will be created. Custom exceptions can be passed in, but the default retries are on SocketTimeoutException and SSLException. This change integrates this input stream implementation with S3InputStream, but RetryableINputStream should be able to be used for the other input streams implementations that are provided by Iceberg.

This change relies on the Failsafe dependency.

amogh-jahagirdar · 2024-06-03T19:05:06Z

aws/src/test/java/org/apache/iceberg/aws/s3/TestFuzzyS3InputStream.java

+  public void testReadWithFuzzyStreamRetrySucceed(IOException exception) throws Exception {
+    testRead(
+        fuzzyStreamClient(new AtomicInteger(3), exception), new S3FileIOProperties(), DATA_SIZE);
+  }


This test takes way too long. That's primarily because it internally tests retrying on just read() (the non-buffered reads) which means every byte read will fail and be retried 2 times with a 500 ms in between. So essentially that's a second per byte.

I think what we can do is modularize further and only do buffered read tests + with a much smaller data size test the per byte read to exercise that code path in the tests. The buffered read tests are pretty fast.

amogh-jahagirdar · 2024-06-03T19:06:04Z

aws/src/test/java/org/apache/iceberg/aws/s3/TestFuzzyS3InputStream.java

+import software.amazon.awssdk.services.s3.model.PutObjectRequest;
+import software.amazon.awssdk.services.s3.model.PutObjectResponse;
+
+public class TestFuzzyS3InputStream extends TestS3InputStream {


I copied a lot of the test logic from https://github.com/apache/iceberg/pull/4912/files, will mark @jackye1995 as coauthor

I saw @xiaoxuandev also had the same tests in #8221 so I'm marking her as co-author here as well

amogh-jahagirdar · 2024-06-03T19:10:21Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

-    return s3.getObject(requestBuilder.build(), ResponseTransformer.toInputStream());
+    stream =
+        RetryableInputStream.builderFor(
+                () -> s3.getObject(requestBuilder.build(), ResponseTransformer.toInputStream()))


any failures that occur on the retry during the getObject request (not the stream read) should just be handled by the SDK. Don't think we need to add anything custom for that since the S3 client is already pluggable.

I feel like there's an issue here. When the stream gets recreated, the stream will reset back to the original position and we continue from there as if we're at the right place in the stream.

The pos won't reflect the new position of the stream, if I'm reading this correctly. I would think the following retry would need to start from next to reflect where the next read should start. There's a small problem with the single byte read method because we increment the positions prior to read, so that would likely need to be adjusted to be after the read like in the range read method.

I'll double check this, I think you're right although I'm not sure why the first test case wouldn't surface that (since the content would be different). We may need to seek properly on the input stream initialization during retry.

I revisited this PR, and took a look with fresh eyes and yes the current logic is definitely incorrect for the case where a non range read (readFully, readTail perform range reads) is performed.

For the range-reads we don't really care about the current position for the purpose of tracking in the retryable input stream. But for normal seeking based reads we definitely do!

I think the way to solve this is to pass a supplier of the current position to the retryable input stream. That supplier would have a reference to this and the stream provider would be a function which accepts a position. Upon retries the stream provider would open a new connection and open a stream that begins with the position that the position supplier (which is guaranteed to be the correct position to start the stream from) returns.

I updated this, the RetryableInputStream offers two builder APIs, one for specifying just a new stream initialization and one for a stream initialization plus a position supplier. The stream initialization function takes in a position (the position can be null to handle the range based requests since for range reads with explicit begin/end we don't care about the current position in the stream). cc @danielcweeks

Discussed with @danielcweeks , for the range read cases since we're using IOUtil readFully/readRemaining which will read the range in a buffered manner. On retries we would read from the beginning position but the internal stream tracking in readFully/readRemaining would not reset to the right position in the buffer to read so that's still an issue.

What we can do is to just not retry the RangeReadable methods for now since they're not actually exercised anywhere. Down the line, we could just use FailSafe and retry on the whole method.

gradle/libs.versions.toml

amogh-jahagirdar · 2024-06-03T19:14:58Z

gradle/libs.versions.toml

@@ -37,6 +37,7 @@ delta-standalone = "3.1.0"
 delta-spark = "3.2.0"
 esotericsoftware-kryo = "4.0.3"
 errorprone-annotations = "2.27.0"
+failsafe = "3.3.2"


This dependency is quite nice in that it's 0 dependency itself, has Apache licensing and I think there's more use cases in Iceberg to leverage it. For example, I think a lot of the complex logic in Tasks can be simplified.

Furthermore, there's some custom retry logic in JDBC connector which we couldn't use tasks for, but now we could use Failsafe. Wonder what others think

I like this a lot

api/src/main/java/org/apache/iceberg/io/RetryableInputStream.java

SandeepSinghGahir · 2024-08-11T08:09:08Z

When will this merged? I'm getting this issue while reading iceberg tables in glue.

amogh-jahagirdar

When will this merged? I'm getting this issue while reading iceberg tables in glue.

Sorry about that @SandeepSinghGahir , I drafted this PR and never followed through. I just took a pass and determined next steps so that we can take this to completion since I know a few folks are hitting this issue and I think it's reasonable that Iceberg S3 input stream has some level of retries when reading from the stream.

I'll also mention, it's really important for changes like this which are on the critical path that they are well tested so I'm also thinking through the test cases as well.

amogh-jahagirdar · 2024-08-14T02:26:19Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

-    return s3.getObject(requestBuilder.build(), ResponseTransformer.toInputStream());
+    stream =
+        RetryableInputStream.builderFor(
+                () -> s3.getObject(requestBuilder.build(), ResponseTransformer.toInputStream()))


I revisited this PR, and took a look with fresh eyes and yes the current logic is definitely incorrect for the case where a non range read (readFully, readTail perform range reads) is performed.

For the range-reads we don't really care about the current position for the purpose of tracking in the retryable input stream. But for normal seeking based reads we definitely do!

I think the way to solve this is to pass a supplier of the current position to the retryable input stream. That supplier would have a reference to this and the stream provider would be a function which accepts a position. Upon retries the stream provider would open a new connection and open a stream that begins with the position that the position supplier (which is guaranteed to be the correct position to start the stream from) returns.

api/src/main/java/org/apache/iceberg/io/RetryableInputStream.java

core/src/main/java/org/apache/iceberg/io/RetryableInputStream.java

aws/src/test/java/org/apache/iceberg/aws/s3/TestFlakyS3InputStream.java

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

SandeepSinghGahir · 2024-09-19T19:07:21Z

@amogh-jahagirdar Any tentative timeline on merging of this PR?

aws/src/test/java/org/apache/iceberg/aws/s3/TestS3InputStream.java

Co-authored-by: Jack Ye <[email protected]> Co-authored-by: Xiaoxuan Li <[email protected]>

danielcweeks

Thanks for all the work and revisions @amogh-jahagirdar!

amogh-jahagirdar · 2024-09-24T16:26:29Z

Thanks for the reviews @danielcweeks! Merging.

tedyu · 2024-10-03T22:31:21Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

+      RetryPolicy.builder()
+          .handle(
+              ImmutableList.of(
+                  SSLException.class, SocketTimeoutException.class, SocketException.class))


Should software.amazon.awssdk.core.exception.SdkClientException be included in the exception list ?
It indicates issues with the client-side networking stack, such as network timeouts.

Judging by the comment in the SdkClientException class, it should not be retryable as there might be multiple reasons for it

Base type for all client exceptions thrown by the SDK. This exception is thrown when service could not be contacted for a response, or when client is unable to parse the response from service. Exceptions that extend SdkClientException are assumed to be not retryable, with a few exceptions: RetryableException - usable when calls should explicitly be retried Exceptions mentioned as a retryable exception in SdkDefaultRetrySetting See Also: SdkServiceException

Is there a more specific one that you were thinking?

Looking at https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/core/internal/retry/SdkDefaultRetrySetting.html, we can add RETRYABLE_EXCEPTIONS to the list.

tedyu · 2024-10-03T22:39:05Z

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java

+              ImmutableList.of(
+                  SSLException.class, SocketTimeoutException.class, SocketException.class))
+          .onFailure(failure -> openStream(true))
+          .withMaxRetries(3)


It seems we can use jitter so that simultaneous retries don't overwhelm the system:

.withBackoff(1, 10, TimeUnit.SECONDS, BackoffJitter.random())

EqualJitterBackoffStrategy adds the jitter internallly on every retry: software.amazon.awssdk.core.retry.backoff.EqualJitterBackoffStrategy

Scratch my response above, it won't apply here since this happens outside of SDK

Co-authored-by: Jack Ye <[email protected]> Co-authored-by: Xiaoxuan Li <[email protected]>

github-actions bot added API build AWS labels Jun 3, 2024

amogh-jahagirdar force-pushed the retry-reading-input-stream branch from 096a4f1 to f4788e7 Compare June 3, 2024 19:07

amogh-jahagirdar commented Jun 3, 2024

View reviewed changes

gradle/libs.versions.toml Outdated Show resolved Hide resolved

amogh-jahagirdar commented Jun 3, 2024

View reviewed changes

singhpk234 reviewed Jun 3, 2024

View reviewed changes

api/src/main/java/org/apache/iceberg/io/RetryableInputStream.java Outdated Show resolved Hide resolved

amogh-jahagirdar mentioned this pull request Jun 7, 2024

Core: Use Failsafe in ClientPoolImpl retry logic #10458

Closed

singhpk234 mentioned this pull request Jun 9, 2024

S3 InputsStream: Reopen connection on Connection Reset #10470

Closed

amogh-jahagirdar added this to the Iceberg 1.6.0 milestone Jun 10, 2024

amogh-jahagirdar removed this from the Iceberg 1.6.0 milestone Jun 21, 2024

amogh-jahagirdar commented Aug 14, 2024

View reviewed changes

amogh-jahagirdar force-pushed the retry-reading-input-stream branch from f4788e7 to deae6eb Compare August 21, 2024 02:27

github-actions bot added core and removed API labels Aug 21, 2024

amogh-jahagirdar force-pushed the retry-reading-input-stream branch 9 times, most recently from 28513fc to 8ba83e3 Compare August 22, 2024 00:13

amogh-jahagirdar marked this pull request as ready for review August 22, 2024 00:13

amogh-jahagirdar requested review from danielcweeks and singhpk234 August 22, 2024 00:13

amogh-jahagirdar force-pushed the retry-reading-input-stream branch 2 times, most recently from 9707998 to 3b916fe Compare September 17, 2024 18:47

danielcweeks reviewed Sep 17, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/io/RetryableInputStream.java Outdated Show resolved Hide resolved

amogh-jahagirdar force-pushed the retry-reading-input-stream branch 2 times, most recently from d77f22d to 56cef08 Compare September 17, 2024 19:59

amogh-jahagirdar commented Sep 17, 2024

View reviewed changes

aws/src/test/java/org/apache/iceberg/aws/s3/TestFlakyS3InputStream.java Show resolved Hide resolved

amogh-jahagirdar commented Sep 17, 2024

View reviewed changes

aws/src/test/java/org/apache/iceberg/aws/s3/TestFlakyS3InputStream.java Show resolved Hide resolved

amogh-jahagirdar force-pushed the retry-reading-input-stream branch 3 times, most recently from 95fb440 to f65a26d Compare September 17, 2024 21:13

amogh-jahagirdar commented Sep 17, 2024

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java Outdated Show resolved Hide resolved

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java Show resolved Hide resolved

amogh-jahagirdar force-pushed the retry-reading-input-stream branch from f65a26d to ffb1274 Compare September 17, 2024 21:21

amogh-jahagirdar commented Sep 17, 2024

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java Show resolved Hide resolved

amogh-jahagirdar changed the title ~~API, AWS: Add RetryableInputStream and use that in S3InputStream~~ API, AWS: Retry S3InputStream reads Sep 17, 2024

amogh-jahagirdar force-pushed the retry-reading-input-stream branch 5 times, most recently from 3dbdf1e to 9d81f18 Compare September 23, 2024 16:46

amogh-jahagirdar commented Sep 23, 2024

View reviewed changes

aws/src/test/java/org/apache/iceberg/aws/s3/TestS3InputStream.java Show resolved Hide resolved

API, AWS: Add RetryableInputStream and use that in S3InputStream

3421040

Co-authored-by: Jack Ye <[email protected]> Co-authored-by: Xiaoxuan Li <[email protected]>

amogh-jahagirdar force-pushed the retry-reading-input-stream branch from 9d81f18 to 3421040 Compare September 23, 2024 17:29

danielcweeks approved these changes Sep 23, 2024

View reviewed changes

amogh-jahagirdar merged commit c0d73f4 into apache:main Sep 24, 2024
50 checks passed

tedyu reviewed Oct 3, 2024

View reviewed changes

edgarRd mentioned this pull request Oct 16, 2024

AWS: Fix S3InputStream retry policy #11335

Merged

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

API, AWS: Retry S3InputStream reads (apache#10433)

90163ae

Co-authored-by: Jack Ye <[email protected]> Co-authored-by: Xiaoxuan Li <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API, AWS: Retry S3InputStream reads #10433

API, AWS: Retry S3InputStream reads #10433

amogh-jahagirdar commented Jun 3, 2024 •

edited

Loading

amogh-jahagirdar Jun 3, 2024 •

edited

Loading

amogh-jahagirdar Jun 3, 2024

amogh-jahagirdar Aug 22, 2024

amogh-jahagirdar Jun 3, 2024

danielcweeks Jun 3, 2024

amogh-jahagirdar Jun 3, 2024

amogh-jahagirdar Aug 14, 2024

amogh-jahagirdar Aug 22, 2024

amogh-jahagirdar Sep 17, 2024

amogh-jahagirdar Jun 3, 2024

nastra Aug 22, 2024

SandeepSinghGahir commented Aug 11, 2024

amogh-jahagirdar left a comment

amogh-jahagirdar Aug 14, 2024

SandeepSinghGahir commented Sep 19, 2024

danielcweeks left a comment

amogh-jahagirdar commented Sep 24, 2024

tedyu Oct 3, 2024

ookumuso Oct 3, 2024

tedyu Oct 3, 2024

tedyu Oct 3, 2024

ookumuso Oct 3, 2024 •

edited

Loading

API, AWS: Retry S3InputStream reads #10433

API, AWS: Retry S3InputStream reads #10433

Conversation

amogh-jahagirdar commented Jun 3, 2024 • edited Loading

amogh-jahagirdar Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandeepSinghGahir commented Aug 11, 2024

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandeepSinghGahir commented Sep 19, 2024

danielcweeks left a comment

Choose a reason for hiding this comment

amogh-jahagirdar commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ookumuso Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

amogh-jahagirdar commented Jun 3, 2024 •

edited

Loading

amogh-jahagirdar Jun 3, 2024 •

edited

Loading

ookumuso Oct 3, 2024 •

edited

Loading