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: Make sure overridden configurations are applied #11274

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

hsiang-c
Copy link
Contributor

@hsiang-c hsiang-c commented Oct 7, 2024

Context

  • In the iceberg-aws module, the community has added the following S3 configurations over time:
S3 configuration Pull Request
Signer configuration #7562
UserAgent configuration #9963
Retry configuration #11052
  • As PR AWS: Make sure Signer + User Agent config are both applied #10198 suggested, calling builder.overrideConfiguration() will return a new ClientOverrideConfiguration instance and ignore existing overridden configurations.
  • However, the previous PR didn't take care of the Retry configuration b/c it was introduced only a few days ago.

Proposed changes

  • Make sure we can apply the following configurations without losing conf values.
s3Properties.applySignerConfiguration(builder);
s3Properties.applyUserAgentConfigurations(builder);
s3Properties.applyRetryConfigurations(builder);

@github-actions github-actions bot added the AWS label Oct 7, 2024
@hsiang-c
Copy link
Contributor Author

hsiang-c commented Oct 7, 2024

@ookumuso @nastra Please review the fix when you have time, thanks.

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for catching and fixing this @hsiang-c

@nastra nastra merged commit 745e819 into apache:main Oct 7, 2024
49 checks passed
@hsiang-c
Copy link
Contributor Author

hsiang-c commented Oct 7, 2024

Thank you for your review @nastra

@hsiang-c hsiang-c deleted the keep_overridden_conf branch October 7, 2024 12:28
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.

2 participants