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(storage): azdls #223

Merged
merged 12 commits into from
Aug 2, 2024
Merged

feat(storage): azdls #223

merged 12 commits into from
Aug 2, 2024

Conversation

twuebi
Copy link
Contributor

@twuebi twuebi commented Jul 31, 2024

azure datalake storage support, currently relies on forks of iceberg-rust -> opendal -> reqsign

we're also pulling in a lot of azure sdk dependencies just for the sas token stuff + validating the storage profile, once FileIO is up to speed we may wanna reconsider this, reqsign also has some sas facilities.

@twuebi twuebi marked this pull request as draft July 31, 2024 10:59
@twuebi twuebi changed the title wip: azdls storage feat(storage): azdls Aug 1, 2024
@twuebi twuebi marked this pull request as ready for review August 1, 2024 10:41
@@ -9,6 +10,14 @@ pub(crate) async fn write_metadata_file(
table_metadata: impl Serialize,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, ich glaube hier sollten wir nochmal ran in einem nächsten PR.
Die metadata_location ist ja schon Teil der table_metadata. Die aktuelle Implementierung lässt Fehler zu.
Darüber hinaus werden wir in Zukunft auch properties brauchen. Ein Kollege schaut sich gerade #156 an (@gongrilla), wofür wir properties brauchen.

crates/iceberg-catalog/src/catalog/io.rs Outdated Show resolved Hide resolved
crates/iceberg-catalog/src/service/storage/az.rs Outdated Show resolved Hide resolved
crates/iceberg-catalog/src/service/storage/az.rs Outdated Show resolved Hide resolved
crates/iceberg-catalog/src/service/storage/az.rs Outdated Show resolved Hide resolved
crates/iceberg-catalog/src/service/storage/az.rs Outdated Show resolved Hide resolved
crates/iceberg-catalog/src/service/storage/az.rs Outdated Show resolved Hide resolved
crates/iceberg-catalog/src/service/storage/az.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@c-thiel c-thiel left a comment

Choose a reason for hiding this comment

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

We should also update the openapi-spec.

crates/iceberg-catalog/src/service/storage/mod.rs Outdated Show resolved Hide resolved
crates/iceberg-catalog/src/service/storage/az.rs Outdated Show resolved Hide resolved
crates/iceberg-catalog/src/service/storage/az.rs Outdated Show resolved Hide resolved
crates/iceberg-catalog/src/service/storage/az.rs Outdated Show resolved Hide resolved
c-thiel
c-thiel previously approved these changes Aug 2, 2024
crates/iceberg-catalog/src/service/storage/error.rs Outdated Show resolved Hide resolved
table_location,
secret
.ok_or_else(|| {
CredentialsError::MissingCredential(self.storage_type())
Copy link
Contributor

Choose a reason for hiding this comment

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

By doing this we rule out the possibility of a credential-free adls - completely public. I am not sure if this is relevant- would the storage driver support it?

Copy link
Contributor Author

@twuebi twuebi Aug 2, 2024

Choose a reason for hiding this comment

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

Reqsign blows up if there's no credential, even worse, it'll take whatever it can find in env if nothing is passed to it which may cause really weird behavior if some az creds end up mounted into a catalog container, something like a profile being created for a warehouse based on the catalog provider's az account instead of from some client's account.

@twuebi twuebi enabled auto-merge (squash) August 2, 2024 14:19
@twuebi twuebi merged commit fd11428 into main Aug 2, 2024
14 checks passed
@twuebi twuebi deleted the tp/azdls-storage branch August 2, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants