Decouple ObjectStore from Reqwest#7183
Conversation
tustvold
left a comment
tustvold
left a comment
There was a problem hiding this comment.
The vast majority of this PR is fairly mechanical, but I've highlighted some key areas
|
|
||
| /// The [`Body`] of an [`HttpRequest`] | ||
| #[derive(Debug, Clone)] | ||
| pub struct HttpRequestBody(Inner); |
There was a problem hiding this comment.
We define fixed body types as we need HttpClient to be dyn-compatible (it also makes things slightly easier to follow)
| } | ||
| } | ||
|
|
||
| pub(crate) struct HttpRequestBuilder { |
There was a problem hiding this comment.
This is a crate-private builder that provides an interface similar to reqwest but which can then be used with the HttpClient. Unfortunately reqwest::Request is largely opaque and I couldn't see an obvious way to make use of it.
| } | ||
| } | ||
|
|
||
| pub(crate) fn add_query_pairs<I, K, V>(uri: &mut Uri, query_pairs: I) |
There was a problem hiding this comment.
It is a shame that http::Request uses http::Uri which lacks the ergonomic niceities of Url, so we require some additional shenanigans
|
|
||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| #[non_exhaustive] | ||
| pub enum HttpErrorKind { |
There was a problem hiding this comment.
This allows the retry logic to work on top of arbitrary HttpClients
| inner: RequestError, | ||
| } | ||
|
|
||
| impl std::fmt::Display for RetryError { |
There was a problem hiding this comment.
This somewhat lays the ground work for #6377
| } | ||
|
|
||
| impl RetryExt for reqwest::RequestBuilder { | ||
| impl RetryExt for HttpRequestBuilder { |
There was a problem hiding this comment.
We could theoretically remove the extension trait, as HttpRequestBuilder, is not a foreign type. I opted to keep it for now though.
| }; | ||
|
|
||
| // Reqwest error variants aren't great, attempt to refine them | ||
| let mut source = e.source(); |
There was a problem hiding this comment.
This logic is moved out of retry.rs and reframed in terms of HttpErrorKind
|
Is it right to say that instead of
? I wonder if we need the custom service type though or if we could use something like |
|
Also I think the keyword that is used by other libs for this kind of refactor is called "sans-io", i.e. to make no assumption about the underlying IO layer or framework. |
I think we just depend on
I went back and forth on this, the reason for preferring a custom service was the following:
Ultimately defining a very simple async_trait in terms of |
| /// An HTTP protocol error | ||
| /// | ||
| /// Clients should return this when an HTTP request fails to be completed, e.g. because | ||
| /// of a connection issue. This does **not** include HTTP requests that are return |
There was a problem hiding this comment.
FWIW the way reqwest conflates these two scenarios I have always found immensely confusing
object_store/src/client/retry.rs
Outdated
| HttpErrorKind::Connect | HttpErrorKind::Request => true, // Request not sent, can retry | ||
| HttpErrorKind::Timeout => is_idempotent, | ||
| HttpErrorKind::Unknown | ||
| | HttpErrorKind::Interrupted |
There was a problem hiding this comment.
Note: I think we should retry Interrupted if request is_idempotent
crepererum
left a comment
crepererum
left a comment
There was a problem hiding this comment.
I think we could merge it as it is, but I would like to see a proper "don't use private interfaces" decoupling of reqwest to make sure that all other implementations are also 1st-class citizens.
| [features] | ||
| default = ["fs"] | ||
| cloud = ["serde", "serde_json", "quick-xml", "hyper", "reqwest", "reqwest/json", "reqwest/stream", "chrono/serde", "base64", "rand", "ring"] | ||
| cloud = ["serde", "serde_json", "quick-xml", "hyper", "reqwest", "reqwest/stream", "chrono/serde", "base64", "rand", "ring", "dep:http", "http-body-util", "form_urlencoded", "serde_urlencoded"] |
There was a problem hiding this comment.
I think we can do that in a follow-up, but it might be good to settle on the explicit style for all references, i.e. prefixing all dependency crates with dep:.
|
|
||
| impl HttpConnector for ReqwestConnector { | ||
| fn connect(&self, options: &ClientOptions) -> crate::Result<HttpClient> { | ||
| let client = options.client()?; |
There was a problem hiding this comment.
It seems like reqwest is still a bit of a special snowflake and has special integration w/ object_store. I wonder if it would help to move ALL reqwest implementations into a single submodule and make it an optional dependency. Ideally the reqwest impls. should also NOT use any pub(super)/pub(crate)/private methods. This of course could only be enforced if we move the reqwest-related code into a new crate, but I until object_store doesn't have it's own repo (ref #6183) I think this is going to be too messy.
There was a problem hiding this comment.
#7203 makes reqwest an optional feature, and shows that the only area it actually leaks into the public API is through some certificate shenanigans on ClientOptions (that can just be gated by the feature).
* Create HttpClient * WIP * Decouple from reqwest * Hook up HttpConnector * WIP * Update other stores * Format * Remove out of date comment * RAT * Test fixes * Lints * Fix feature flags
2 participants
Which issue does this PR close?
Rationale for this change
See tickets
What changes are included in this PR?
Are there any user-facing changes?