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

Add async-std support and replaced reqwest with surf (#58) #72

Merged
merged 5 commits into from
Nov 12, 2020
Merged

Add async-std support and replaced reqwest with surf (#58) #72

merged 5 commits into from
Nov 12, 2020

Conversation

JEnoch
Copy link
Contributor

@JEnoch JEnoch commented Nov 4, 2020

Description

This PR replaces reqwest usage with surf usage that supports both tokio and async-std. It also modifies the tests to run with both.
Fixes #58.

Checklist

  • Formatted code using cargo fmt --all
  • Linted code using clippy cargo clippy --all-targets --all-features -- -D warnings
  • Updated README.md using cargo readme -r influxdb -t ../README.tpl > README.md
  • Reviewed the diff. Did you leave any print statements or unnecessary comments?
  • Any unfinished work that warrants a separate issue captured in an issue with a TODO code comment

@JEnoch JEnoch changed the title Add async-std support and replaced reqwest with surf (#58) Add async-std support and replaced reqwest with surf (Empty2k12/influxdb-rust#58) Nov 4, 2020
@JEnoch JEnoch changed the title Add async-std support and replaced reqwest with surf (Empty2k12/influxdb-rust#58) Add async-std support and replaced reqwest with surf (#58) Nov 4, 2020
@Empty2k12
Copy link
Collaborator

Thanks for this PR!

Could we add two flags (async-std and tokio) and have tokio being the default one. If tokio is enabled, the hyper backend of surf is used, and if async-std is enabled, the curl backend is used?

Also, I'd like to merge #73 first, so the changed calls can be ported over to surf as well.

@JEnoch
Copy link
Contributor Author

JEnoch commented Nov 6, 2020

I'm not sure to understand the purpose of adding async-std and tokio features. Your influxdb-rust library is actually agnostic to async-std or tokio, as surf is as well.
I made some tests with surf and you mixing hyper-client or curl-client with either async-std or tokio for the binary's main() works in all combinations.

But you're right about giving the user the possibility to choose the http backend.
If you agree, I add in this PR the same features than surf in influxdb-rust (See https://github.com/http-rs/surf/blob/main/Cargo.toml#L19). And set hyper-client as default if you prefer.

@JEnoch
Copy link
Contributor Author

JEnoch commented Nov 9, 2020

Hi @Empty2k12,

I updated this PR with latest status, merging with #73.
I also added the features to choose the http library used by surf (default to hyper-client), following my proposal above.

Note that switching to surf I get a 50% perf improvement on the bench pushed in #73 and no request errors (I had a lot when using reqwest) !

@JEnoch
Copy link
Contributor Author

JEnoch commented Nov 12, 2020

Hi @Empty2k12 ,

Any concern or issue reviewing this PR ?

@Empty2k12
Copy link
Collaborator

Sorry, I'm quite busy with university right now. Tested your PR and will merge it now.
Thanks for this contribution! 🚀

@Empty2k12 Empty2k12 merged commit cc8750a into influxdb-rs:master Nov 12, 2020
@JEnoch
Copy link
Contributor Author

JEnoch commented Nov 12, 2020

Thanks for the merge! (and sorry for the "harassment" 😉 )
Do you plan a release and publication to crates.io any time soon ?

@Empty2k12
Copy link
Collaborator

@JEnoch Yes, I plan releasing this as 0.3.0.

@Empty2k12
Copy link
Collaborator

@JEnoch The version is now available under 0.3.0 at crates.io: https://crates.io/crates/influxdb

Thanks again for your contribution!

@JEnoch
Copy link
Contributor Author

JEnoch commented Nov 16, 2020

@Empty2k12 That's great, thanks a lot! I just made eclipse-zenoh to use it (eclipse-zenoh/zenoh@a206fa83).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whether to support async-std?
2 participants