You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "edgarRd (via GitHub)" <gi...@apache.org> on 2024/10/16 21:13:41 UTC
[PR] AWS: Fix S3inputstream retry policy [iceberg]
edgarRd opened a new pull request, #11335:
URL: https://github.com/apache/iceberg/pull/11335
The retry policy for `S3InputStream` reads introduced in https://github.com/apache/iceberg/commit/c0d73f4ef5c16401bdfd62e1745faf2fbbf62177 is not actually re-opening the stream on each retry attempt given that it uses `onFailure` which, as per the [documentation](https://failsafe.dev/javadoc/core/dev/failsafe/PolicyBuilder.html#onFailure-dev.failsafe.event.EventListener-), it is triggered after the whole (retry) policy has failed completely and was unable to produce a successful result, i.e. all retries have been exhausted and failed. With the existing implementation, once a stream fails with something like `SSLException` due to a socked connection reset, the retries will keep failing (due to not actually reopening the stream) and therefore this won't work. The `onFailure` call does not work either because by the time `openStream` is called, the retry policy failure has already been propagated, failing the read operation.
The tests also were not testing that the retry policy actually called `openStream(true)` to reset the stream, it only tested that the calls went through the retry policy a given number of times, generally always succeeding on the last one.
This PR fixes the problem by using `onRetry` instead, as per the [documentation](https://failsafe.dev/javadoc/core/dev/failsafe/RetryPolicyBuilder.html#onRetry-dev.failsafe.event.EventListener-), to re-open the stream right before we attempt the read again. Also, it fixes the tests by actually checking that the stream is reset (re-opened).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3InputStream retry policy [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#discussion_r1803860896
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestFlakyS3InputStream.java:
##########
@@ -49,10 +51,29 @@
public class TestFlakyS3InputStream extends TestS3InputStream {
+ private AtomicInteger resetForRetryCounter;
+
+ @BeforeEach
+ public void setupTest() {
+ resetForRetryCounter = new AtomicInteger(0);
+ }
+
+ @Override
+ S3InputStream newInputStream(S3Client s3Client, S3URI uri) {
+ return new S3InputStream(s3Client, uri) {
+ @Override
+ void resetForRetry() throws IOException {
+ resetForRetryCounter.incrementAndGet();
+ super.resetForRetry();
+ }
Review Comment:
Thanks for fixing this, this was crucial for verifying the retry behavior actually resets the input stream. Seems like we were just getting lucky on the tests before since they only counted the number of attempts but not what was actually happening *in* the attempts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3InputStream retry policy [iceberg]
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on code in PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#discussion_r1803882204
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestFlakyS3InputStream.java:
##########
@@ -49,10 +51,29 @@
public class TestFlakyS3InputStream extends TestS3InputStream {
+ private AtomicInteger resetForRetryCounter;
+
+ @BeforeEach
+ public void setupTest() {
+ resetForRetryCounter = new AtomicInteger(0);
+ }
+
+ @Override
+ S3InputStream newInputStream(S3Client s3Client, S3URI uri) {
+ return new S3InputStream(s3Client, uri) {
+ @Override
+ void resetForRetry() throws IOException {
+ resetForRetryCounter.incrementAndGet();
+ super.resetForRetry();
+ }
Review Comment:
Yes. That's correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3InputStream retry policy [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#discussion_r1803860896
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestFlakyS3InputStream.java:
##########
@@ -49,10 +51,29 @@
public class TestFlakyS3InputStream extends TestS3InputStream {
+ private AtomicInteger resetForRetryCounter;
+
+ @BeforeEach
+ public void setupTest() {
+ resetForRetryCounter = new AtomicInteger(0);
+ }
+
+ @Override
+ S3InputStream newInputStream(S3Client s3Client, S3URI uri) {
+ return new S3InputStream(s3Client, uri) {
+ @Override
+ void resetForRetry() throws IOException {
+ resetForRetryCounter.incrementAndGet();
+ super.resetForRetry();
+ }
Review Comment:
Thanks for fixing this, this was crucial for verifying the retry behavior actually resets the input stream. Seems like we were just getting lucky on the tests before since they only counted the number of attempts but not what was happening *in* the attempts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3InputStream retry policy [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#discussion_r1803860090
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java:
##########
@@ -66,10 +70,14 @@ class S3InputStream extends SeekableInputStream implements RangeReadable {
private int skipSize = 1024 * 1024;
private RetryPolicy<Object> retryPolicy =
RetryPolicy.builder()
- .handle(
- ImmutableList.of(
- SSLException.class, SocketTimeoutException.class, SocketException.class))
- .onFailure(failure -> openStream(true))
+ .handle(RETRYABLE_EXCEPTIONS)
+ .onRetry(
+ e -> {
+ LOG.warn(
+ "Retrying read from S3, reopening stream (attempt {})", e.getAttemptCount());
+ resetForRetry();
+ })
+ .onFailure(e -> LOG.error("Failed to read from S3 after retry policy", e.getException()))
Review Comment:
"Failed to read from S3 input stream after exhausting all retries"
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestFlakyS3InputStream.java:
##########
@@ -49,10 +51,29 @@
public class TestFlakyS3InputStream extends TestS3InputStream {
+ private AtomicInteger resetForRetryCounter;
+
+ @BeforeEach
+ public void setupTest() {
+ resetForRetryCounter = new AtomicInteger(0);
+ }
+
+ @Override
+ S3InputStream newInputStream(S3Client s3Client, S3URI uri) {
+ return new S3InputStream(s3Client, uri) {
+ @Override
+ void resetForRetry() throws IOException {
+ resetForRetryCounter.incrementAndGet();
+ super.resetForRetry();
+ }
Review Comment:
Thanks for fixing this, this was crucial for verifying the retry behavior actually resets the input stream. Seems like we were just getting lucky on the tests before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3InputStream retry policy [iceberg]
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on code in PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#discussion_r1803884005
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java:
##########
@@ -66,10 +70,14 @@ class S3InputStream extends SeekableInputStream implements RangeReadable {
private int skipSize = 1024 * 1024;
private RetryPolicy<Object> retryPolicy =
RetryPolicy.builder()
- .handle(
- ImmutableList.of(
- SSLException.class, SocketTimeoutException.class, SocketException.class))
- .onFailure(failure -> openStream(true))
+ .handle(RETRYABLE_EXCEPTIONS)
+ .onRetry(
+ e -> {
+ LOG.warn(
+ "Retrying read from S3, reopening stream (attempt {})", e.getAttemptCount());
+ resetForRetry();
+ })
+ .onFailure(e -> LOG.error("Failed to read from S3 after retry policy", e.getException()))
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3InputStream retry policy [iceberg]
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#issuecomment-2419753940
Thanks for the reviews, @amogh-jahagirdar @singhpk234 and @Parth-Brahmbhatt !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3inputstream retry policy [iceberg]
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#issuecomment-2417977467
@amogh-jahagirdar @jackye1995 @danielcweeks PTAL. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3InputStream retry policy [iceberg]
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#issuecomment-2418102961
@amogh-jahagirdar I've address the comment. PTAL - Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3InputStream retry policy [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#issuecomment-2419600165
Thanks @edgarRd, I'll go ahead and merge. Thank you @singhpk234 for reviewing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3InputStream retry policy [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #11335:
URL: https://github.com/apache/iceberg/pull/11335
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Fix S3inputstream retry policy [iceberg]
Posted by "edgarRd (via GitHub)" <gi...@apache.org>.
edgarRd commented on PR #11335:
URL: https://github.com/apache/iceberg/pull/11335#issuecomment-2417977679
cc @Parth-Brahmbhatt
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org