Skip to content

AWS: abort S3 input stream on close if not EOS #7262

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danielcweeks merged 7 commits into from
Apr 4, 2023
Merged

AWS: abort S3 input stream on close if not EOS #7262

Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3939b55
AWS: abort S3 input stream on close if not EOS
bryanck Apr 1, 2023
cf247eb
Close the stream for backwards compatibility
bryanck Apr 2, 2023
492c419
undo unrelated change
bryanck Apr 2, 2023
ac2e5df
add trace log
bryanck Apr 2, 2023
d365d91
comment update
bryanck Apr 2, 2023
69d8f1a
logger updates
bryanck Apr 3, 2023
f881681
handle connection closed exception
bryanck Apr 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import software.amazon.awssdk.core.sync.ResponseTransformer;
import software.amazon.awssdk.http.Abortable;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.GetObjectRequest;
import software.amazon.awssdk.services.s3.model.NoSuchKeyException;
Expand Down Expand Up @@ -194,9 +195,28 @@ private void openStream() throws IOException {
}
}

private void closeStream() throws IOException {
private void closeStream() {
if (stream != null) {
stream.close();
// if we aren't at the end of the stream, and the stream is abortable, then
// call abort() so we don't read the remaining data with the Apache HTTP client
abortStream();
try {
stream.close();
} catch (Exception e) {
// close quietly, closing an aborted stream will throw a content length

@jackye1995 jackye1995 Apr 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I guess I did not express it clearly. What I meant was that, can we check for the exception that if it is a content length check exception then we can skip, otherwise we log a warning?

@bryanck bryanck Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. ContentLengthInputStream is an Apache HTTP class, so referring to that will tie S3InputStream to that client.

@bryanck bryanck Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could check the name of the class, but that seems like a little bit of a hack.

@jackye1995 jackye1995 Apr 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, what does the exception look like? I though there are some error code that we can get out of the exception that would imply this specific error. Is that not there?

@bryanck bryanck Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what gets thrown. Calling close on the aborted stream will attempt a read which then throws that.

@jackye1995 jackye1995 Apr 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking ConnectionClosedException seems good enough to me, because it makes sense that if the connection is already closed then we can omit the error.

@jackye1995 jackye1995 Apr 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think org.apache.http.ConnectionClosedException is already in the class path given we have some REST client integration in core, but I might be wrong.

@bryanck bryanck Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I pushed that change

@bryanck bryanck Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a different version than being used by the AWS SDK (v5 instead of v4). Also, the AWS bundle shades the library.

@bryanck bryanck Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is we check which HTTP client is being used and change the close behavior based on that.

// check exception with the Apache HTTP client, which is expected
}
stream = null;
}
}

private void abortStream() {
try {
if (stream instanceof Abortable && stream.read() != -1) {

@jackye1995 jackye1995 Apr 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we want to read one more byte here? It might cause one more request. I think it does not hurt to even abort when it's already fully read.

@bryanck bryanck Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you abort the connection then it will be invalidated and removed from the pool, so it won't be reused. So this is an optimization to attempt to reuse the connection in cases where it has been fully read.

((Abortable) stream).abort();
}
} catch (Exception e) {
LOG.warn("An error occurred while aborting the stream", e);
}
}

Expand Down