amogh-jahagirdar commented on code in PR #10433:
URL: https://github.com/apache/iceberg/pull/10433#discussion_r1762163236
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java:
##########
@@ -178,18 +183,23 @@ private void positionStream() throws IOException {
}
private void openStream() throws IOException {
- GetObjectRequest.Builder requestBuilder =
- GetObjectRequest.builder()
- .bucket(location.bucket())
- .key(location.key())
- .range(String.format("bytes=%s-", pos));
-
- S3RequestUtil.configureEncryption(s3FileIOProperties, requestBuilder);
-
closeStream();
try {
- stream = s3.getObject(requestBuilder.build(),
ResponseTransformer.toInputStream());
+ stream =
+ RetryableInputStream.builderFor(
+ rangeStart -> {
+ GetObjectRequest.Builder requestBuilder =
+ GetObjectRequest.builder()
+ .bucket(location.bucket())
+ .key(location.key())
+ .range(String.format("bytes=%s-", rangeStart));
+ S3RequestUtil.configureEncryption(s3FileIOProperties,
requestBuilder);
+ return s3.getObject(
+ requestBuilder.build(),
ResponseTransformer.toInputStream());
+ },
+ () -> pos)
Review Comment:
Discussed with @danielcweeks, we can just simplify this to remove the extra
position supplier parameter to the builderFor API and just pass in the pos as
part of the stream supplier
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java:
##########
@@ -139,7 +140,11 @@ private InputStream readRange(String range) {
S3RequestUtil.configureEncryption(s3FileIOProperties, requestBuilder);
- return s3.getObject(requestBuilder.build(),
ResponseTransformer.toInputStream());
+ stream =
+ RetryableInputStream.builderFor(
+ () -> s3.getObject(requestBuilder.build(),
ResponseTransformer.toInputStream()))
Review Comment:
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.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
