Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: support passing customized connector or connector adapter #4286

Closed
wenym1 opened this issue Feb 28, 2024 · 4 comments
Closed

Comments

@wenym1
Copy link

wenym1 commented Feb 28, 2024

Motivation

We are using AWS S3 or any S3 compatible object store as our storage backend. Previously we use aws-sdk-s3 to access the object store, and recently we are migrating from the aws sdk to opendal.

Previously when we use the aws sdk, it supports passing customized http client (see method http_client). In our code, we build our own http client to apply some customized logic. Most logics can be supported in opendal, such as setting nodelay, keepalive. The only thing left is passing a customized connector. When building a hyper client by the build method, it support passing a customized connector that implements the Connect trait. The trait is accessed when creating a new connection. We leverage this feature to monitor the tcp connection, such as number of existing connection, throughput of each connection etc. We implemented our own connector that wraps the original hyper http connector and impl the Connect trait in risingwavelabs/risingwave#11848. However, in opendal this feature is not supported yet.

Requested feature

I think there are two ways to support the feature.

The first one is to support passing a customized http client or a connector like what the aws sdk does. This may leak too much flexibility to our external users, as it depends on users to properly initialize the connector.

The second one is to support passing a connector adapter with the following definition

trait ConnectorAdapter {
    fn wrap(connector: impl Connect) -> impl Connect;
}

With this adapter trait, the opendal can initial the connector internally, and then wrap the initialized connector with this adapter, and in the end pass it to the http client.

Implementation-wise consideration

It seems that opendal is using reqwest to make the http request. I took a rough look at the code of reqwest. Though its underlying http client is also hyper, in here, it seems that the connector used by the http client is fixed. So to support this feature, we shall either modify the reqwest code to support passing a customized connector, or change to make the http request from the raw hyper client.

@Xuanwo
Copy link
Member

Xuanwo commented Feb 28, 2024

Thanks for raising the issue!

I'm open to switching our HTTP client from reqwest to hyper, which offers us greater control over the underlying HTTP operations.

I can see the following benefits:

  • Upgrade to hyper 1 for now (good to our users dependences)
  • More control on http (like in mentioned by OP)
  • Async Runtime Agnostic

Based on our current codebase, here are several notes:

Good

OpenDAL wraps all http client side code in http_util, we don't need to replace every usage of http client.

Bad

OpenDAL now relies on the following features provided by reqwest:

  • system proxy (should be simple I guess)
  • HTTP Upgrade (from http/1.1 to http/2)
  • HTTP 1.1 Keep Alive (a.k.a connection pool)

@Xuanwo
Copy link
Member

Xuanwo commented Apr 10, 2024

I think about this again and plan to introduce a HttpClient trait:

trait HttpClient {
  async fn send(&self, req: Request<Buffer>) -> Response<Buffer>;
}

You can do whatever you want to the client itself (you can even use the same client from aws sdk!).

What do you think? @wenym1

@wenym1
Copy link
Author

wenym1 commented Apr 11, 2024

I think about this again and plan to introduce a HttpClient trait:

trait HttpClient {
  async fn send(&self, req: Request<Buffer>) -> Response<Buffer>;
}

You can do whatever you want to the client itself (you can even use the same client from aws sdk!).

What do you think? @wenym1

Looks promising to me!

If you have any PR on it, could you invite me to review so that I can try whether the new trait can satisfy our usage? Thanks!

@Xuanwo
Copy link
Member

Xuanwo commented Oct 16, 2024

Hi @wenym1, this feature has been implemented as seen in #4471. We plan to include it in v0.50.1, our upcoming release.

@Xuanwo Xuanwo closed this as completed Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants