BEAC-167: Move object store configs to object_store_factory#580
BEAC-167: Move object store configs to object_store_factory#580lizardoluis merged 10 commits intomainfrom
Conversation
lizardoluis
requested review from
gruuya and
mildbyte
and removed request for
mildbyte
| itertools = ">=0.10.0" | ||
| object_store = { version = "0.10.2", features = ["aws", "azure", "gcp"] } | ||
|
|
||
| serde = "1.0.156" |
There was a problem hiding this comment.
We should use this in [dependencies] below.
There was a problem hiding this comment.
@gruuya I get an error if I do not set it here, as I'm inheriting it in the object_store_factory Cargo.toml. Btw, it is also set in the [dependecies].
The error is:
error: failed to load manifest for workspace member `/Users/lizardo/Workspace/seafowl/object_store_factory`
referenced by workspace at `/Users/lizardo/Workspace/seafowl/Cargo.toml`
Caused by:
failed to parse manifest at `/Users/lizardo/Workspace/seafowl/object_store_factory/Cargo.toml`
Caused by:
error inheriting `serde` from workspace root manifest's `workspace.dependencies.serde`
Caused by:
`dependency.serde` was not found in `workspace.dependencies
There was a problem hiding this comment.
No, i meant for serde to remain in workspace.dependencies, just to change line 129/131 in this file to inherit from it serde = { workspace = true }.
| #[cfg(feature = "object-store-s3")] | ||
| S3(S3), | ||
| #[cfg(feature = "object-store-gcs")] |
There was a problem hiding this comment.
We should probably get rid of these two features in Cargo.toml (not used in the new configs, and I don't think we ever opt-out of these).
There was a problem hiding this comment.
I also purged some occurrences of it in the code.
src/config/schema.rs
Outdated
| fn test_parse_config_invalid_s3() { | ||
| let error = | ||
| load_config_from_string(TEST_CONFIG_INVALID_S3, false, None).unwrap_err(); | ||
| println!("##### ERROR: {:?}", error.to_string()); |
There was a problem hiding this comment.
| println!("##### ERROR: {:?}", error.to_string()); |
src/object_store/wrapped.rs
Outdated
| let prefix = match self.config.clone() { | ||
| ObjectStoreConfig::AmazonS3(aws_config) => aws_config.prefix, | ||
| ObjectStoreConfig::GoogleCloudStorage(google_config) => google_config.prefix, | ||
| _ => None, | ||
| }; | ||
|
|
||
| match prefix { | ||
| Some(prefix) => Path::from(format!("{prefix}/{table_prefix}")), | ||
| None => Path::from(table_prefix), | ||
| } |
There was a problem hiding this comment.
Seems simpler to do it with one pattern match as before?
| let prefix = match self.config.clone() { | |
| ObjectStoreConfig::AmazonS3(aws_config) => aws_config.prefix, | |
| ObjectStoreConfig::GoogleCloudStorage(google_config) => google_config.prefix, | |
| _ => None, | |
| }; | |
| match prefix { | |
| Some(prefix) => Path::from(format!("{prefix}/{table_prefix}")), | |
| None => Path::from(table_prefix), | |
| } | |
| match self.config.clone() { | |
| ObjectStoreConfig::AmazonS3(S3Config { | |
| prefix: Some(prefix), | |
| .. | |
| }) | |
| | ObjectStoreConfig::GoogleCloudStorage(GCSConfig { | |
| prefix: Some(prefix), | |
| .. | |
| }) => Path::from(format!("{prefix}/{table_prefix}")), | |
| _ => Path::from(table_prefix), | |
| } |
There was a problem hiding this comment.
I reverted, though I think the readability was better with the change.
2 participants
This PR moves the object store configuration structs into the new crate
object_store_factory.