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

feat(client): Implement Tower's Service for Client #1747

Closed
wants to merge 1 commit into from

Conversation

davidbarsky
Copy link
Contributor

@davidbarsky davidbarsky commented Jan 16, 2019

This is relatively small. Open questions:

  • Should this behind a feature flag?
    • Until DirectService is the default in Tower?
    • ...past that point?

I think additional middleware implementations should be tower-rs/tower-hyper, such as specific timeouts and retry policies. Building on that, I wonder if it makes sense to have a “batteries-included” middleware crate—I know my employer has specific best-practices around retries that are typically followed-company wide, but that probably doesn't belong in that crate.

Still to do before merging:

  • Documentation and examples for Tower in Hyper

@davidbarsky
Copy link
Contributor Author

Oh, I also have no idea how to tackle replacing Service's Service impl. That's a bit... beyond me.

@seanmonstar
Copy link
Member

The possibility of needing to go to tower_service = "0.3" relatively soon worries me some...

@sfackler
Copy link
Contributor

sfackler commented Jan 16, 2019

New cargo can handle that by supporting multiple optional dependencies of tower at different versions. Might be worth doing this in a separate crate with a newtype wrapper until hyper can bump its minimum supported Rust version to 1.31.

@seanmonstar
Copy link
Member

@sfackler wait what? Got an example, or a link to something I can read more about?

@sfackler
Copy link
Contributor

Here's an example in rust-postgres: https://github.com/sfackler/rust-postgres/blob/master/tokio-postgres/Cargo.toml#L58

While we don't do it yet, we could have an equivalent bit-vec-06 or whatever dependency that works side-by-side with the existing one.

@davidbarsky
Copy link
Contributor Author

I'll close this ticket until tower-rs/tower#136 is resolved. I think it's best that any experimentation along these lines occur in tower-rs/tower-hyper.

@davidbarsky davidbarsky deleted the impl-service-for-client branch January 22, 2019 21:47
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

Successfully merging this pull request may close these issues.

3 participants