Skip to content
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.

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

Update S3 retry logic to account for the underlying cause in case of IOException #15238

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

abhishekrb19
Copy link
Contributor

@abhishekrb19 abhishekrb19 commented Oct 24, 2023

In some places we wrap AmazonS3Exception inside an IOException and all IOExceptions by default are retried by the S3 retry logic. For example, a 403 AccessDenied Se exception code wrapped inside IOException shouldn't be retried.

This PR updates the s3 retry logic to account for the underlying cause if it's found.

Release note

S3 exceptions like 403 AccessDenied codes wrapped inside other exceptions, like IOException, won't trigger unnecessary retries.


Key changed/added classes in this PR
  • S3Utils.java
  • S3UtilsTest.java
  • S3DataSegmentPullerTest.java

This PR has:

  • been self-reviewed.
  • a release note entry in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

…ption.

4xx and other errors wrapped in IOException for instance aren't retriable.
object0.setBucketName(bucket);
object0.setKey(keyPrefix + "/renames-0.gz");
object0.getObjectMetadata().setLastModified(new Date(0));
object0.setObjectContent(new FileInputStream(tmpFile));

Check warning

Code scanning / CodeQL

Potential input resource leak Warning test

This FileInputStream is not always closed on method exit.

final File tmpFile = temporaryFolder.newFile("gzTest.gz");

try (OutputStream outputStream = new GZIPOutputStream(new FileOutputStream(tmpFile))) {

Check warning

Code scanning / CodeQL

Potential output resource leak Warning test

This FileOutputStream is not always closed on method exit.
Copy link
Contributor

@zachjsh zachjsh left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Contributor

@amaechler amaechler left a comment

Choose a reason for hiding this comment

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

Nice! 🙇🐻

@abhishekrb19 abhishekrb19 merged commit 63e3e95 into apache:master Oct 24, 2023
53 checks passed
@abhishekrb19 abhishekrb19 deleted the fixup_s3_retry branch October 24, 2023 22:04
CaseyPan pushed a commit to CaseyPan/druid that referenced this pull request Nov 17, 2023
…`IOException` (apache#15238)

* Update S3 retry logic based on the underlying cause in case of IOException.

4xx and other errors wrapped in IOException for instance aren't retriable.

* Fix CI
@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants