RetryPolicy refactor suggestions #1045
Merged
Conversation
This file contains 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
hawkw
approved these changes
Jun 14, 2021
Comment on lines
+21
to
+24
| /// Constructs a boxing layer that erases the inner request type with [`EraseRequest`]. | ||
| pub fn erased() -> impl layer::Layer<S, Service = EraseRequest<S>> + Clone + Copy { | ||
| EraseRequest::layer() | ||
| } |
| /// An extension to [`tower::retry::Policy`] that adds a method to prepare a | ||
| /// request to be retried, possibly changing its type. | ||
| pub trait RetryPolicy<Req, Res, E>: tower::retry::Policy<Self::RetryRequest, Res, E> { | ||
| pub trait PrepareRequest<Req, Res, E>: tower::retry::Policy<Self::RetryRequest, Res, E> { |
There was a problem hiding this comment.
my original motivation for naming this RetryPolicy --- which i agree is a bit confusing --- was because it requires tower's RetryPolicy trait, so when it's used, we only bound the type with this trait.
i also considered the name PrepareRequest, but it seemed kind of weird to me to require that the policy implement PrepareRequest and not mention that it is also a retry policy? maybe that's not actually worth worrying about, and i'm fine with this naming scheme if you think it's clearer...
There was a problem hiding this comment.
could call it PreparePolicy or something like that -- but RetryPolicy vs Policy is a bit too subtle imo
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A few suggestions on #1043
EraseRequestviaBoxRequest::erased-- we don't actually care about the different types--the behavior is basically the same -- there's just some minor differences with regards to preserving marker types. It reads a little bit more naturally if we just hang it all off BoxRequest.linkerd_retry::RetryPolicytrait tolinkerd_retry::PrepareRequest(to match its only method). It's a bit... malkovich to have bothtower::retry::Policyandlinkerd_retry::RetryPolicywhich are closely related but different.linkerd_retrynow re-exports relevanttower::retrytypes.app::core::retryto reflect actual types. For instance,NewRetryis nowNewRetryPolicy-- since it actually implementsNewPolicyand has nothing to do with theretry::NewRetrytype.NewRetryLayereliminated in favor of an anonymous type.inlineson pass-through service/proxy methods