chore: break Glue support into its own crate without rusoto #1825
Conversation
|
I just have a comment about the crate name. Is the goal to have |
|
@cmackenzie1 Yes the plan is to put each catalog provider into its own crate, there's not really any common code between something like Unity, which will likely have dependencies on Databricks SDKs, and Glue which has AWS SDK dependencies. Some of the theory between parcelling these things out is that we can rev releases of these less frequently and use semver to make it easier to pull in updates. |
rtyler
force-pushed
the
datacatalog-glue-1601
branch
from
3e31e5b to
47cb7db
Compare
|
The benchmark test failure I don't understand at all, but also don't think it has any relation to this pull request 😕 |
| /// Error from a specific catalog provider | ||
| #[error("Catalog implementation error: {0}")] | ||
| Error(Box<dyn std::error::Error + Send + Sync + 'static>), | ||
|
|
There was a problem hiding this comment.
should integrations raise the above Generic error, which also contains an identifier from the specific catalogue? This pattern worked quite nicely in object store :).
There was a problem hiding this comment.
Can you link me to what you're referring to @roeap ? I can make the changes here once I understand what you're suggesting 😄
There was a problem hiding this comment.
certainly :).
delta-rs/crates/deltalake-core/src/data_catalog/mod.rs
Lines 25 to 31 in 2b913b3
| glue = ["deltalake-core/glue"] | ||
| glue-native-tls = ["deltalake-core/glue-native-tls"] | ||
| glue = ["deltalake-catalog-glue"] | ||
| glue-native-tls = ["deltalake-catalog-glue/native-tls"] |
There was a problem hiding this comment.
was just wondering if there maybe is a way to propagate this from the general native-tls feature, and if this would even be wanted?.
There was a problem hiding this comment.
I have not found a good way to propagate features, it's a little tedious book-keeping 😢
There was a problem hiding this comment.
not entirely sure, but I found the example below.
[dependencies]
serde = { version = "1.0.133", optional = true }
rgb = { version = "0.8.25", optional = true }
[features]
serde = ["dep:serde", "rgb?/serde"]The docs there say..
In this example, enabling the serde feature will enable the serde dependency. It will also enable the serde feature for the rgb dependency, but only if something else has enabled the rgb dependency.
So if I read that correctly, we may be able to add "deltalake-catalog-glue?/native-tls" to the native-tls feature and it only takes effect, if glue is enabled?
There was a problem hiding this comment.
Coming back to this, I think this is a good idea, but it will require a features rework in the metacarte since there we also have s3-native-tls for example. I'll tackle that in a follow up pull request
rtyler
force-pushed
the
datacatalog-glue-1601
branch
from
47cb7db to
716f20e
Compare
rtyler
force-pushed
the
datacatalog-glue-1601
branch
from
716f20e to
a99c4d3
Compare
|
@rtyler - meant to resolve merge conflicts, but broke formatting :( |
| let catalog = GlueDataCatalog::from_env() | ||
| .await | ||
| .expect("Failed to load catalog from the environment"); | ||
| println!("caralog: {catalog:?}"); |
There was a problem hiding this comment.
| println!("caralog: {catalog:?}"); | |
| println!("catalog: {catalog:?}"); |
🤷
This change also pilots a removal of Rusoto in favor of the AWS SDK for Rust which AWS is now supporting and funding the development of. The API surface is largely the same, but this move I believe will aso ensure that we're much more consistent on handling AWS environment variables for some things. Related to delta-io#1601
rtyler
force-pushed
the
datacatalog-glue-1601
branch
2 times, most recently
from
bfd8d9b to
8ceca20
Compare
This change also pilots a removal of Rusoto in favor of the AWS SDK for Rust which AWS is now supporting and funding the development of.
The API surface is largely the same, but this move I believe will aso ensure that we're much more consistent on handling AWS environment variables for some things.
Related to #1601