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

feat: override dynamodb config #3011

Merged
merged 5 commits into from
Nov 23, 2024

Conversation

thomas-chauvet
Copy link
Contributor

Description

This PR add the possibility to override dynamoDB configuration via storage_options:

  • AWS_ENDPOINT_URL_DYNAMODB overrides AWS_ENDPOINT_URL,
  • AWS_REGION_DYNAMODB overrides AWS_REGION,
  • AWS_ACCESS_KEY_ID_DYNAMODB overrides AWS_ACCESS_KEY_ID,
  • AWS_SECRET_ACCESS_KEY_DYNAMODB overrides AWS_SECRET_ACCESS_KEY.

It solves cases where we use an S3-compatible storage without mutual exclusion but we want to use DynamoDB anyway as a locking provider.
It also solves the case where we want to pass different credentials or region for S3 and dynamoDB.

Related Issue(s)

Documentation

Documentation has been updated to explain how to override configuration.

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Nov 21, 2024
@rtyler rtyler added this to the v0.22 milestone Nov 21, 2024
@rtyler rtyler enabled auto-merge November 21, 2024 21:50
rtyler
rtyler previously approved these changes Nov 21, 2024
auto-merge was automatically disabled November 22, 2024 06:33

Head branch was pushed to by a user without write access

@thomas-chauvet thomas-chauvet force-pushed the feat/override-dynamodb-config branch from 04340b7 to d04e317 Compare November 22, 2024 06:35
@thomas-chauvet
Copy link
Contributor Author

@rtyler thanks for your quick review! I've just fixed a small equality in test. I've made a workaround before and I've left it in the code. It is now properly fixed. Thanks!

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 86.61417% with 17 lines in your changes missing coverage. Please review.

Project coverage is 72.42%. Comparing base (0d076a7) to head (2d1b6f6).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/aws/src/lib.rs 87.37% 12 Missing and 1 partial ⚠️
crates/aws/src/logstore/dynamodb_logstore.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3011      +/-   ##
==========================================
- Coverage   72.43%   72.42%   -0.02%     
==========================================
  Files         128      128              
  Lines       40859    40974     +115     
  Branches    40859    40974     +115     
==========================================
+ Hits        29597    29675      +78     
- Misses       9357     9374      +17     
- Partials     1905     1925      +20     

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


🚨 Try these New Features:

@ion-elgreco
Copy link
Collaborator

@thomas-chauvet @rtyler This might be short lived, if we can get delta-rs bumped soon to the new objectstore release. I would like us to default using putIfAbsent by default for any S3 implementation, then it's up to users who use S3 which does not support this, to set copyIfNotExists or allow unsafe_rename

@thomas-chauvet
Copy link
Contributor Author

Does this mean that locking provider will be definitely remove from delta-rs?

@ion-elgreco
Copy link
Collaborator

Does this mean that locking provider will be definitely remove from delta-rs?

Not per se, but the value of a locking provider is quite minimal unless you still use an S3 implementation like LakeFS that doesn't have these request headers yet

@rtyler
Copy link
Member

rtyler commented Nov 22, 2024

@ion-elgreco I don't expect the DynamoDB log store code to go away for at least 3-6 months, even if we had the putIfAbsent support today, I would want to keep the support around because existing deployments using DynamoDB will not all be ready to migrate 100%.

I see value in adding these parameters until such a time when all DynamoDB code is removed from delta-rs, which currently has no scheduled time.

@ion-elgreco
Copy link
Collaborator

@ion-elgreco I don't expect the DynamoDB log store code to go away for at least 3-6 months, even if we had the putIfAbsent support today, I would want to keep the support around because existing deployments using DynamoDB will not all be ready to migrate 100%.

I see value in adding these parameters until such a time when all DynamoDB code is removed from delta-rs, which currently has no scheduled time.

For sure, we should keep it for some time!

@rtyler rtyler enabled auto-merge November 23, 2024 14:24
@rtyler rtyler added this pull request to the merge queue Nov 23, 2024
Merged via the queue into delta-io:main with commit 68d57ef Nov 23, 2024
24 checks passed
@thomas-chauvet thomas-chauvet deleted the feat/override-dynamodb-config branch November 25, 2024 06:32
@thomas-chauvet
Copy link
Contributor Author

Thanks for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate
Projects
None yet
3 participants