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.

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

Jump to bottom

fix: aws credential resolving, reduce api calls #3107

Merged
merged 2 commits into from
Jan 8, 2025
Merged

fix: aws credential resolving, reduce api calls #3107

merged 2 commits into from
Jan 8, 2025
+71 −63

Conversation

ion-elgreco
Copy link
Collaborator

@ion-elgreco ion-elgreco commented Jan 8, 2025 •
edited
Loading

Description

  • Adds a trait to logstore and objectstore factory to parse options
  • LogStoreFactory gets correct credentials as well now
  • Reduces latency by only construction SDK config once for object store
  • Fixes is_aws by also checking the env vars for locking provider and force_aws_credential_load, tested it and doesn't show aws sdk config logs anymore when you use endpoint url

cc @rtyler

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Jan 8, 2025
@ion-elgreco ion-elgreco force-pushed the fix/s3_integration branch 2 times, most recently from 9ed9c10 to f33c9af Compare January 8, 2025 12:14
@ion-elgreco ion-elgreco requested a review from fvaleye as a code owner January 8, 2025 12:18
@github-actions github-actions bot added the binding/python Issues for the Python package label Jan 8, 2025
@ion-elgreco ion-elgreco closed this Jan 8, 2025
@github-actions github-actions bot removed binding/rust Issues for the Rust crate binding/python Issues for the Python package labels Jan 8, 2025
@ion-elgreco ion-elgreco reopened this Jan 8, 2025
@github-actions github-actions bot added binding/python Issues for the Python package binding/rust Issues for the Rust crate labels Jan 8, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 8, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 81.13208% with 10 lines in your changes missing coverage. Please review.

Project coverage is 72.39%. Comparing base (053601b) to head (1424e57).

Files with missing lines Patch % Lines
crates/aws/src/storage.rs 81.63% 7 Missing and 2 partials ⚠️
crates/aws/src/lib.rs 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3107      +/-   ##
==========================================
+ Coverage   72.34%   72.39%   +0.05%     
==========================================
  Files         128      128              
  Lines       41296    41302       +6     
  Branches    41296    41302       +6     
==========================================
+ Hits        29875    29900      +25     
+ Misses       9504     9500       -4     
+ Partials     1917     1902      -15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rtyler rtyler self-assigned this Jan 8, 2025
@rtyler rtyler added this to the v0.23 milestone Jan 8, 2025
@ion-elgreco ion-elgreco force-pushed the fix/s3_integration branch 2 times, most recently from fc5eca8 to 1424e57 Compare January 8, 2025 13:51
@ion-elgreco ion-elgreco enabled auto-merge January 8, 2025 14:04
hntd187
hntd187 reviewed Jan 8, 2025
View reviewed changes
crates/aws/src/storage.rs Outdated
for (os_key, os_value) in std::env::vars_os() {
if let (Some(key), Some(value)) = (os_key.to_str(), os_value.to_str()) {
if let Ok(config_key) = AmazonS3ConfigKey::from_str(&key.to_ascii_lowercase()) {
if !options.0.contains_key(config_key.as_ref()) {
Copy link
Collaborator

@hntd187 hntd187 Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

look into the entry api for hashmaps you could simplify this code with it

https://doc.rust-lang.org/beta/std/collections/hash_map/struct.HashMap.html#method.entry

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll do that in a follow up!

@rtyler rtyler force-pushed the fix/s3_integration branch from 1424e57 to 829c720 Compare January 8, 2025 15:33
@ion-elgreco ion-elgreco added this pull request to the merge queue Jan 8, 2025
Merged via the queue into delta-io:main with commit 0b90a11 Jan 8, 2025
22 checks passed
@ion-elgreco ion-elgreco deleted the fix/s3_integration branch January 8, 2025 17:34
This was referenced Jan 9, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hntd187 hntd187 hntd187 left review comments

@rtyler rtyler rtyler approved these changes

@wjones127 wjones127 Awaiting requested review from wjones127 wjones127 is a code owner

@roeap roeap Awaiting requested review from roeap roeap is a code owner

@fvaleye fvaleye Awaiting requested review from fvaleye fvaleye is a code owner

Assignees

@rtyler rtyler

Labels
binding/python Issues for the Python package binding/rust Issues for the Rust crate
Projects
None yet
Milestone
v0.23
Development

Successfully merging this pull request may close these issues.
3 participants
@ion-elgreco @rtyler @hntd187