-
-
Notifications
You must be signed in to change notification settings - Fork 319
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
Upgrades Tower to 0.5.1 #1589
Upgrades Tower to 0.5.1 #1589
Conversation
f884bc6
to
e97b01e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1589 +/- ##
=======================================
- Coverage 75.3% 75.3% -0.0%
=======================================
Files 82 82
Lines 7327 7328 +1
=======================================
Hits 5515 5515
- Misses 1812 1813 +1
|
e97b01e
to
14465ac
Compare
fixes kube-rs#1569 Signed-off-by: Mark Ingram <[email protected]>
14465ac
to
6dd1938
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this. From a quick pass this looks sensible to both me and CI.
// - `BoxService` for dynamic response future type | ||
inner: Buffer<BoxService<Request<Body>, Response<Body>, BoxError>, Request<Body>>, | ||
// - `BoxFuture` for dynamic response future type | ||
inner: Buffer<Request<Body>, BoxFuture<'static, Result<Response<Body>, BoxError>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does seem like a nicer signature split. Interesting that it has to be 'static though. Is that a problem you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BoxService
can only be created by moving the underlying Service - https://docs.rs/tower/latest/tower/util/struct.BoxService.html#impl-BoxService%3CT,+U,+E%3E whereas a BoxFuture
can be created from a ref, hence generic over lifetime.
The kube::Client
design owns the Buffer and to make Buffer Clone
the Future needs to be 'static
- https://tower-rs.github.io/tower/tower/buffer/struct.Buffer.html#impl-Clone-for-Buffer%3CReq,+F%3E
So 'static
here is both unavoidable & fine (IMO!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar in the Tonic upgrade PR - https://github.com/hyperium/tonic/pull/1892/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks for explaining!
fixes #1569
Motivation
Solution
hyper-util 0.1.9 unblocked this change by dropping the tower 0.4 dep -> hyperium/hyper-util#151
tower 0.5 includes this change to make Buffer generic over the Future rather than the Service -> tower-rs/tower#654
tower 0.5 includes this change to rework Either to no longer change the error type to BoxError, meaning Kube-RS now has to do so itself -> tower-rs/tower#637
tower-http 0.6.1 includes this change which promises to improve (de)compression -> tower-rs/tower-http#521