Xuanwo commented on code in PR #5184:
URL: https://github.com/apache/opendal/pull/5184#discussion_r1802394032
##########
core/src/raw/http_util/client.rs:
##########
@@ -20,21 +20,26 @@ use std::fmt::Formatter;
use std::future;
use std::mem;
use std::str::FromStr;
+use std::sync::Arc;
use futures::TryStreamExt;
use http::Request;
use http::Response;
+use once_cell::sync::Lazy;
use raw::oio::Read;
use super::parse_content_encoding;
use super::parse_content_length;
use super::HttpBody;
use crate::*;
+/// Http client used across opendal for loading credentials.
+pub static CREDENTIAL_CLIENT: Lazy<reqwest::Client> =
Lazy::new(reqwest::Client::new);
Review Comment:
How about naming it `GLOBAL_REQWEST_CLIENT`?
Please add in comments that this is merely a temporary solution because
`reqsign` requires a `reqwest` client to be passed. We will remove it after the
next major version of `reqsign`, which will enable users to provide their own
client.
##########
core/src/raw/http_util/client.rs:
##########
@@ -47,26 +52,24 @@ impl Debug for HttpClient {
impl HttpClient {
/// Create a new http client in async context.
pub fn new() -> Result<Self> {
- Self::build(reqwest::ClientBuilder::new())
+ let fetcher = Arc::new(ReqwestHttpFetcher::new());
+ Ok(Self { fetcher })
}
/// Construct `Self` with given [`reqwest::Client`]
- pub fn with(client: reqwest::Client) -> Self {
- Self { client }
+ pub fn with(client: impl HttpFetch + 'static) -> Self {
Review Comment:
We can remove `'static`.
##########
core/src/raw/http_util/mod.rs:
##########
@@ -24,6 +24,7 @@
mod client;
pub use client::HttpClient;
+pub use client::CREDENTIAL_CLIENT;
Review Comment:
Please mark this as `pub(crate)`. I prefer not to expose it to our users,
and it's highly likely that we'll remove it in the future.
##########
core/src/raw/http_util/client.rs:
##########
@@ -78,6 +81,34 @@ impl HttpClient {
/// Fetch a request in async way.
pub async fn fetch(&self, req: Request<Buffer>) ->
Result<Response<HttpBody>> {
+ self.fetcher.fetch(req).await
+ }
+}
+
+#[async_trait::async_trait]
+pub trait HttpFetch: Send + Sync {
+ /// Fetch a request in async way.
+ async fn fetch(&self, req: Request<Buffer>) -> Result<Response<HttpBody>>;
+}
+
+#[derive(Clone)]
+struct ReqwestHttpFetcher {
Review Comment:
Maybe we can implement `HttpFetch` for `reqwest::Client` directly?
##########
core/src/raw/http_util/client.rs:
##########
@@ -78,6 +81,34 @@ impl HttpClient {
/// Fetch a request in async way.
pub async fn fetch(&self, req: Request<Buffer>) ->
Result<Response<HttpBody>> {
+ self.fetcher.fetch(req).await
+ }
+}
+
+#[async_trait::async_trait]
Review Comment:
Let's implement it in the same way as we do for `oio::Read`:
https://github.com/apache/opendal/blob/9f5f3ce00812b0a07e7648fac66bc4a01c1048d9/core/src/raw/oio/read/api.rs#L44-L105
We will have a `HttpFetch` and `HttpFetchDyn`
##########
core/src/raw/http_util/client.rs:
##########
@@ -78,6 +81,34 @@ impl HttpClient {
/// Fetch a request in async way.
pub async fn fetch(&self, req: Request<Buffer>) ->
Result<Response<HttpBody>> {
+ self.fetcher.fetch(req).await
+ }
+}
+
+#[async_trait::async_trait]
+pub trait HttpFetch: Send + Sync {
Review Comment:
Please also add `Unpin + 'static` here, we don't allow users to implement
`HttpFetch` over ref.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
