feat: implement GetOptions extensions

[waynr]

Which issue does this PR close?

Closes #7155

Rationale for this change

This is laid out in the associated issue.

What changes are included in this PR?

• Introduces a new type, object_store::Extensions
  • Uses a HashMap<TypeId, Arc<dyn Any + Send + Sync + 'static>>
  • This is modeled after (ie copied from) and meant to be compatible with the extension mechanism in datafusion::execution::config::SessionConfig
  • It's probably worth considering using the same double hash avoidance optimization used there, but for simplicity I've left it out of this PR while it's in draft form -- I just wanted to end up with something I could test out and get early feedback on.
  • This is a public type for now because I am not sure what the preference of maintainers will be and it's simpler in my mind for now to make use of this at the datafusion level (draft PR for that coming shortly after this) if we can construct an Extensions instance once rather than on every ObjectStore method call where one is needed.
• Adds a new extensions: Extensions field to GetOptions
  • Could easily be implemented for PutOptions as well, but I'm personally less interested in that so I'm leaving it out for now.

Are there any user-facing changes?

Yes, the new field in GetOptions and a new publicly-exposed type, object_store::Extensions.

[waynr]

By the way, it's worth mentioning that I based this PR on the tag for the 0.11.2 release because I ran into a bunch of breaking changes with respect to integer types in main branch when integrating this change into datafusion (namely this #6961). I think it would be best to rebase this onto main before if it gets approved by maintainers of course.

Also worth noting that this is a draft PR for now because I still need to test it out in https://github.com/influxdata/influxdb, write better docstrings, and run the approach by @crepererum and @tustvold before spending too much additional time on it.

[crepererum]

I would prefer if we don't use Any directly for the extensions but a proper trait. This also allows you to properly implement Debug and even PartialEq:

trait Extension: std::fmt::Debug {
    fn as_any(&self) -> &dyn std::any::Any;
    fn partial_eq(&self, other: &dyn std::any::Any) -> bool;
}

#[derive(Debug, PartialEq)]
struct MyExt {
    x: u8,
}

impl Extension for MyExt {
    fn as_any(&self) -> &dyn std::any::Any {
        self
    }

    fn partial_eq(&self, other: &dyn std::any::Any) -> bool {
        other
            .downcast_ref::<Self>()
            .map(|other| other.partial_eq(self))
            .unwrap_or_default()
    }
}

[waynr]

@crepererum I don't know if this violates the spirit of your preference for making Extension trait implementations also implement PartialEq and Debug, but this was the best I could come up with that would allow me to build a Extensions instance from a HashMap<TypeId, Arc<dyn Any + Send + Sync + 'static>>. I should have a PR up shortly demonstrating the use of this in datafusion.

[waynr]

Here's the PR I mentioned that demonstrates what I had in mind with respect to how to use this: https://github.com/apache/datafusion/pull/14805/files#diff-cd6fee4ac01d3e6f21ca25e40cbaf9a462882eca8ed80d7868079327a9ce31a9R576

I'll test this out in https://github.com/influxdata/influxdb tomorrow to see if I can get the tracing spans I wanted in our caching object store implementation.

[crepererum]

I think this wrapper is flawed. People should implement the trait themselves or -- if they cannot due to the orphan rule -- create a dedicated wrapper type for whatever they need.

[Xuanwo]

Hi, I'm concerned about adding such an API in object_store, especially a model tailored for specific use cases like metrics and tracing.

Have we considered adding a TracingObjectStore that wraps another object store and adds traces for every API call? I think this approach is more extensible and can help avoid making changes for every action.

[waynr]

We have discussed that possibility, but it's not straightforward to do without non-trivial refactors in InfluxDB. And even if we did that, the tracing spans would not be properly parented within the hierarchy of tracing spans -- the best we would be able to do is get them parented at the root-span of a given query (as opposed to under the most recently-created descendant of that root span).

[waynr]

For what it's worth, another use case aside from tracing spans that @crepererum mentioned to me was passing file size hints via GetOptions to a caching layer to avoid extraneous HEAD requests in certain use cases.

[crepererum]

Hi, I'm concerned about adding such an API in object_store, especially a model tailored for specific use cases like metrics and tracing.

What exactly does concern you?

Have we considered adding a TracingObjectStore that wraps another object store and adds traces for every API call? I think this approach is more extensible and can help avoid making changes for every action.

That assumes that you have one single ObjectStore. Truth is, ObjectStore is an abstract interface that lends itself to dynamic composition, e.g. to add caching as wrappers.

[tustvold]

I wonder if supporting PartialEq for extensions is all that important, my understanding is the PartialEq support is a utility for testing. As such I wonder if we just return None if either argument has extensions present?

IMO it would be nice if we didn't have to export an Extension trait publicly and could just support registering anything Send + Sync + Debug, with any necessary trait machinery kept private.

[crepererum]

I wonder if supporting PartialEq for extensions is all that important, my understanding is the PartialEq support is a utility for testing.

Fair, I'm not dying on that hill. std::fmt::Debug would be super useful though.

[crepererum]

IMO it would be nice if we didn't have to export an Extension trait publicly and could just support registering anything Send + Sync + Debug, with any necessary trait machinery kept private.

That can actually be done, but requires some more cleverness. I'll give it a shot.

[crepererum]

I've hacked together an alternative implementation at #7170 which -- I think -- has a reasonable interface.