RetryPolicy refactor suggestions#1045
Conversation
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
lgtm, i'm on board with all these suggestions! i did leave a comment on the trait naming thing explaining my original thought process, but i'm not attached to it at all --- this is probably better. other than that, nothing stood out, thanks!
| /// 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
2 participants
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