[GitHub] [iceberg] danielcweeks commented on a diff in pull request #4912: AWS: add retry logic to S3InputStream

[GitBox <gi...@apache.org>]

danielcweeks commented on code in PR #4912:
URL: https://github.com/apache/iceberg/pull/4912#discussion_r898507252


##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java:
##########
@@ -172,31 +274,62 @@ private void positionStream() throws IOException {
     }
 
     // close the stream and open at desired position
-    LOG.debug("Seek with new stream for {} to offset {}", location, next);
+    LOG.warn("Seek with new stream for {} to offset {}", location, next);
     pos = next;
     openStream();
   }
 
-  private void openStream() throws IOException {
-    GetObjectRequest.Builder requestBuilder = GetObjectRequest.builder()
-        .bucket(location.bucket())
-        .key(location.key())
-        .range(String.format("bytes=%s-", pos));
-
-    S3RequestUtil.configureEncryption(awsProperties, requestBuilder);
-
+  private void openStream() {
     closeStream();
-    stream = s3.getObject(requestBuilder.build(), ResponseTransformer.toInputStream());
+    stream = readRange(String.format("bytes=%s-", pos));
+  }
+
+  private void closeStream() {
+    closeServerSideStream(stream);
+    stream = null;
   }
 
-  private void closeStream() throws IOException {
-    if (stream != null) {
-      stream.close();
+  private static void closeServerSideStream(InputStream streamToClose) {
+    if (streamToClose != null) {
+      try {
+        if (streamToClose instanceof Abortable) {
+          // Stated in the ResponseInputStream javadoc:
+          // If it is not desired to read remaining data from the stream,
+          // you can explicitly abort the connection via abort().
+          ((Abortable) streamToClose).abort();
+        } else {
+          streamToClose.close();
+        }
+      } catch (IOException | AbortedException e) {
+        // ignore failure to abort or close stream
+      }
     }
   }
 
-  public void setSkipSize(int skipSize) {
-    this.skipSize = skipSize;
+  private static boolean shouldRetry(Exception exception) {

Review Comment:
   I think we need to be more explicit about the cases where we do want to retry rather than just defaulting to retry.  I think there were specific cases that were mentined as known issues (e.g. socket timeout).  However, the problem we've had in other retry scenarios is that the retry cases are overly broad and retry when they really shouldn't.  
   
   I think the default here should return false and only return true for the known Exceptions.



-- 
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