kinolaev commented on code in PR #15792:
URL: https://github.com/apache/iceberg/pull/15792#discussion_r3083247455
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3InputStream.java:
##########
@@ -62,4 +66,36 @@ void testReadTailClosesTheStream() throws IOException {
verify(inputStream).close();
}
+
+ @Test
+ void testReadRetriesOnPrematureConnectionClose() throws IOException {
Review Comment:
I added a MockServer and real S3 calls to the test in
c3c43875c8a0ad94ef5ded0970bd76a346d8b952.
I've also found that UrlConnectionHttpClient returns -1 from `read` when the
connection is closed prematurely instead of throwing an exception. That is why
I added `handleResultIf` to the retry policy that checks if `read` returns -1
before the expected end of the stream. The PR now covers both clients' behavior.
Additionally, I've tested both clients with minio in
`aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3InputStream.java`:
```java
public class TestS3InputStream {
// ...
@Test
public void
testReadRetriesOnPrematureConnectionCloseWithApacheHttpClient() throws
Exception {
testReadRetriesOnPrematureConnectionClose(s3, "apache");
}
@Test
public void
testReadRetriesOnPrematureConnectionCloseWithUrlConnectionHttpClient() throws
Exception {
testReadRetriesOnPrematureConnectionClose(s3urlconnection,
"urlconnection");
}
private void testReadRetriesOnPrematureConnectionClose(S3Client s3, String
filename) throws Exception {
S3URI uri = new
S3URI("s3://bucket/path/to/read-retries-on-premature-connection-close-%s.dat".formatted(filename));
byte[] data = randomData(16 * 1024 * 1024);
writeS3Data(uri, data);
S3InputStream stream = newInputStream(s3, uri);
int blockSize = data.length / 2;
byte[] buf1 = new byte[blockSize];
assertThat(stream.readNBytes(buf1, 0, blockSize)).isEqualTo(blockSize);
assertThat(buf1).isEqualTo(Arrays.copyOfRange(data, 0, blockSize));
Thread.sleep(32000);
byte[] buf2 = new byte[blockSize];
assertThat(stream.readNBytes(buf2, 0, blockSize)).isEqualTo(blockSize);
assertThat(buf2).isEqualTo(Arrays.copyOfRange(data, blockSize,
data.length));
// RetryPolicy.builder().onRetry(e -> { attemptCount =
e.getAttemptCount(); })
assertThat(stream.attemptCount).isEqualTo(1);
}
}
```
I didn't commit these tests because on my laptop they only succeed when the
delay between the read calls is greater than 30s. Otherwise there are no
retries (`attemptCount=0`). I'm not sure this specific delay is consistent
enough across different environments to reliably reproduce the behavior and I
didn't want to increase the total execution time.
So, if there are any cases where multiple read calls are separated by a
delay, the PR fixes them.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
