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

[cloud storage] support for reqwest_middleware #229

Closed
nicolas-vivot opened this issue Feb 8, 2024 · 1 comment
Closed

[cloud storage] support for reqwest_middleware #229

nicolas-vivot opened this issue Feb 8, 2024 · 1 comment

Comments

@nicolas-vivot
Copy link
Contributor

nicolas-vivot commented Feb 8, 2024

original "issue"

The storage client do not offer the ability to specify some retry policy.

How to address it

While we can simply do it on our end (wrapping the cloud storage calls in some retry mechanism), one simple way is to address it directly in the cloud storage library by changing the reqwest client being used.

The client currently used for the storage part (and i believe in other modules but i have only looked at the storage part) is reqwest::Client.
Switching to the ClientWithMiddleware from reqwest_middleware allow us to pass a fully customized client by using middlewares.

Benefits doing it

Middle-wares can then be used to extend how requests get processed, without altering the cloud storage crate itself, making it quite flexible.
A couple of examples of middleware that can be used:

  • authentication layer
  • custom retry policy
  • custom retry strategy (when it comes to specific needs or non standard APIs)
  • adding custom headers (for tracing for example)

Changes proposal

I made a change proposal in this PR if you want to have a look at it.

Since the original reqwest crate do not expose trait, the reqwest_middleware client is a wrapper. For this reason, i could not refactor the cloud storage to offer support for both, and had to replace the client types.

In term of support / migration for people who are using the cloud storage library with a custom reqwest::Client, there should not be any issues since there is a From to create a ClientWithMiddleware from an existing reqwest::Client.

notes:

  • the cloud storage crate could also benefit it by moving the custom headers (including the authentication token) out as middlewares.
  • i did not do anything for the default ClientWithMiddleware being built, but i do believe that having one with a default retry strategy would greatly improve the default behavior of the component. I am happy to add it in a separate PR if this one get accepted.
@nicolas-vivot
Copy link
Contributor Author

I am closing this since the PR has been accepted, and it seems like @yoshidan have changed the other modules to also use it.

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

1 participant