Add Builder style config objects for object_store #2204
Conversation
alamb
force-pushed
the
alamb/object_store_config
branch
from
6725394 to
b3c9771
Compare
There was a problem hiding this comment.
I think the diffs in this PR are easier to understand with whitespace blind diff https://github.com/apache/arrow-rs/pull/2204/files?w=1
object_store/src/aws.rs
Outdated
| true, | ||
| ) | ||
| } | ||
| // /// Create a new [`AmazonS3`] that always errors |
There was a problem hiding this comment.
I am not sure what this is used for -- I think perhaps we should remove it. Maybe @tustvold or @carols10cents
remembers what it is for ?
There was a problem hiding this comment.
I seem to remember IOx used it for failing tests, now that ObjectStore is object-safe I'm fairly confident it can go
| @@ -1067,17 +1185,9 @@ mod tests { | |||
|
|
|||
| const NON_EXISTENT_NAME: &str = "nonexistentname"; | |||
|
|
|||
| #[derive(Debug)] | |||
| struct AwsConfig { | |||
There was a problem hiding this comment.
Amusingly (at least to me) the tests had created a partial config struct anyways -- so this PR can be thought of "exposing these configuration options to uers" 😆
| pub struct MicrosoftAzureBuilder { | ||
| account: Option<String>, | ||
| access_key: Option<String>, | ||
| container_name: Option<String>, |
There was a problem hiding this comment.
not sure if I should also call this bucket_name (generic term) or container_name (Azure specific term). I kept the existing naming convention
There was a problem hiding this comment.
I think we should match the convention for Azure, i.e. container_name
object_store/src/aws.rs
Outdated
| true, | ||
| ) | ||
| } | ||
| // /// Create a new [`AmazonS3`] that always errors |
There was a problem hiding this comment.
I seem to remember IOx used it for failing tests, now that ObjectStore is object-safe I'm fairly confident it can go
| pub struct MicrosoftAzureBuilder { | ||
| account: Option<String>, | ||
| access_key: Option<String>, | ||
| container_name: Option<String>, |
There was a problem hiding this comment.
I think we should match the convention for Azure, i.e. container_name
| /// | ||
| /// This allows you to set custom client options such as allowing | ||
| /// non secure connections or custom headers. | ||
| pub fn with_client(mut self, client: Client) -> Self { |
There was a problem hiding this comment.
I wonder if we should keep this pub(crate) at least for now, in case we want to change the client down the line or something...
There was a problem hiding this comment.
Since it is only in test I will make it #[cfg(test)] and we can always make it pub later
alamb
changed the title
|
Benchmark runs are scheduled for baseline = 393f006 and contender = 41d96b2. 41d96b2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2203
Rationale for this change
Current config is both a pain to use as well as will require backwards incompatible changes to extend (e.g. add a new parameter)
What changes are included in this PR?
Add builder style configs:
AmazonS3BuilderMicrosoftAzureBuilderGoogleCloudStorageBuilderAre there any user-facing changes?
Yes, the configuration system is better