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

AWS: avoid static global credentials provider which doesn't play well with lifecycle management #8677

Merged

Conversation

Kontinuation
Copy link
Member

@Kontinuation Kontinuation commented Sep 29, 2023

Fixes #8601

It replaces DefaultCredentialsProvider.create() with DefaultCredentialsProvider.builder().build() to avoid sharing the same credentials provider across multiple clients.

This PR changes 3 places calling DefaultCredentialsProvider.create(), and adds one test for AwsCredentialsProvider.credentialsProvider. The other two are deprecated so no tests were added for them.

…tCredentialsProvider.create()` to avoid sharing the same credentials provider across multiple clients
@github-actions github-actions bot added the AWS label Sep 29, 2023
@Kontinuation Kontinuation changed the title Fix java.lang.IllegalStateException: Connection pool shut down when using web identity token file to authenticate AWS: Fix java.lang.IllegalStateException: Connection pool shut down when using web identity token file to authenticate Sep 29, 2023
@stevenzwu stevenzwu changed the title AWS: Fix java.lang.IllegalStateException: Connection pool shut down when using web identity token file to authenticate Avoid static global credentials provider which doesn't play well with lifecycle management Sep 29, 2023
@stevenzwu stevenzwu changed the title Avoid static global credentials provider which doesn't play well with lifecycle management AWS: avoid static global credentials provider which doesn't play well with lifecycle management Sep 29, 2023
@stevenzwu
Copy link
Contributor

@Kontinuation thx for the fix. LGTM. I updated the subject. will leave it for a couple of days for others to review.

Copy link
Contributor

@dramaticlly dramaticlly left a comment

Choose a reason for hiding this comment

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

thank you for the fix

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

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

I think this is a reasonable fix for the problem at hand. Thanks @Kontinuation !

The only aspect I wonder about is how much benefit we even get from sharing HTTP clients between different service clients (in this case, between STS and S3). The main benefit of sharing HTTP clients is connection pooling; S3 and STS have different endpoints thus sharing HTTP clients between S3 and STS clients won't add any value.

Anyways that's orthogonal to the problem at hand, but just happened to surface to me as part of the debugging of this problem (again thanks @Kontinuation for the in depth analysis of the problem)

@stevenzwu stevenzwu merged commit c68abfc into apache:master Oct 2, 2023
45 checks passed
@stevenzwu
Copy link
Contributor

thanks @Kontinuation for the root cause analysis and the fix. Thanks @dramaticlly @amogh-jahagirdar @danielcweeks for reviews

@nastra nastra added this to the Iceberg 1.4.1 milestone Oct 16, 2023
nastra pushed a commit that referenced this pull request Oct 16, 2023
nastra added a commit that referenced this pull request Oct 18, 2023
… with lifecycle management (#8677) (#8843)

Co-authored-by: Kristin Cowalcijk <bo@wherobots.com>
rodmeneses pushed a commit to rodmeneses/iceberg that referenced this pull request Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

java.lang.IllegalStateException: Connection pool shut down when refreshing table metadata on s3
6 participants