port linkerd2-proxy-error::recover and linkerd2-proxy-resolve to std::future#518
Merged
hawkw merged 6 commits intomaster-tokio-0.2from May 19, 2020
Merged
port linkerd2-proxy-error::recover and linkerd2-proxy-resolve to std::future#518hawkw merged 6 commits intomaster-tokio-0.2from
linkerd2-proxy-error::recover and linkerd2-proxy-resolve to std::future#518hawkw merged 6 commits intomaster-tokio-0.2from
Conversation
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
sadly, recover requires unpin, but i couldn't make it work otherwise Signed-off-by: Eliza Weisman <[email protected]>
olix0r
approved these changes
May 19, 2020
kleimkuhler
reviewed
May 19, 2020
hawkw
added a commit
that referenced
this pull request
May 20, 2020
This branch updates the `linkerd2-proxy-api-resolve` crate to use `std::future` and Tonic. Getting this to work required some upstream changes to linkerd/linkerd2-proxy-api#39 to pass different feature flags to Tonic, but the change here is pretty straigntforward. Depends on #518 and will be rebased onto `master-tokio-02` when that branch merges. Signed-off-by: Eliza Weisman <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch updates the
linkerd2-proxy-resolvecrate tostd::future.It also updates the
recovermodule inlinkerd2-proxy-error, which isa dependency of
resolve, andlinkerd2-reconnect, which depends onrecoverand therefore didn't compile after it was updated.The
linkerd2-proxy-resolve::recovercode currently only works when theunderlying resolver's resolution and future types are both
Unpin, andthe backoff type is also
Unpin. This is because this logic relies abit on being able to move these values in and out of
Options. I triedto implement this without requiring these values to be
Unpin, but theimplementation of the
ResolveFuture, which drives the resolution untilit's connected and then returns it, was more or less impossible to
translate in its current state.
I suspect that this won't be an issue when updating code that depends on
recover— I think the resolutions will probably beUnpinwithout anyintervention from us. If not, we can always either
Box::pinthem, ortry to do a more complete rewrite of this code so that it's refactored
in a way that avoids these issues.
In other cases, we can sometimes just replace code that is difficult to
port to use
async/await. Here, that was not possible, because we areimplementing the
Resolutiontrait, rather than a future. We may wantto consider rewriting
Resolutionto be a trait alias for aStream,so that we can use the
async-streamcrate, but that may have otherdownsides, so I decided to not do that just yet.
Hopefully this all makes sense!