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

Update tower-balance to std::future #335

Merged
merged 9 commits into from
Sep 10, 2019
Merged

Update tower-balance to std::future #335

merged 9 commits into from
Sep 10, 2019

Conversation

jonhoo
Copy link
Collaborator

@jonhoo jonhoo commented Sep 10, 2019

Will rebase and mark as ready once #330 lands (which this depends on)

This also includes a small patch to tower-discover to delegate the Discover trait through Pin<D> where D::Target: Discover (so we can use it through, say, Pin<Box<>>), and a patch to tower-load to make Constant: Debug.

@jonhoo jonhoo requested a review from LucioFranco September 10, 2019 18:29
@@ -88,7 +98,7 @@ where

impl<D, Req> Balance<D, Req>
where
D: Discover,
D: Discover + Unpin,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me really sad, but it is required since we need to call Discover::poll_discover (which takes Pin<&mut Self>) from Service::poll_ready (which takes &mut Self).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also #319

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is fine 👍 can we make sure we have docs on why this is required maybe, just point people to that issue and what they can do to get Unpin (aka box the discover)?

@jonhoo jonhoo force-pushed the jonhoo/update-balance branch from 34ba7be to 1b63bbf Compare September 10, 2019 18:52
@jonhoo jonhoo marked this pull request as ready for review September 10, 2019 18:53
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM no blockers, left some comments inline

tower-balance/src/lib.rs Outdated Show resolved Hide resolved
@@ -88,7 +98,7 @@ where

impl<D, Req> Balance<D, Req>
where
D: Discover,
D: Discover + Unpin,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is fine 👍 can we make sure we have docs on why this is required maybe, just point people to that issue and what they can do to get Unpin (aka box the discover)?

tower-balance/src/pool/mod.rs Show resolved Hide resolved
tower-balance/src/pool/mod.rs Outdated Show resolved Hide resolved
tower-balance/src/pool/test.rs Outdated Show resolved Hide resolved
tower-discover/src/lib.rs Show resolved Hide resolved
@jonhoo jonhoo merged commit 87976ae into v0.3.x Sep 10, 2019
@jonhoo jonhoo deleted the jonhoo/update-balance branch September 11, 2019 15:09
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.

2 participants