tower

tower copied to clipboard

These are lifted out of the test and example code. Two policies are provided:

• RetryLimit, with a bounded number of retry attempts;
• RetryError, with unbounded retry attempts.

Both policies require Req: Clone as it's not possible to write a generic retry policy without being able to clone requests.

The docs should be updated with a little blurb that explains when they should not be used. Also, the Policy docs had an Attempts example corresponding to RetryLimit (formerly Limit in the tests code); I removed it because RetryLimit exists and can be view-sourced.

@hdevalence any interest in wrapping this up, or should we just close the PR?

I'd be happy to wrap this up, but I don't really know what the "good" design would be -- the current PR is just what worked for my needs. But, if there was clarity about what changes would be good, I'd be happy to make them, and I think it would be convenient if something like this was included in Tower.

I'd be happy to wrap this up, but I don't really know what the "good" design would be -- the current PR is just what worked for my needs. But, if there was clarity about what changes would be good, I'd be happy to make them, and I think it would be convenient if something like this was included in Tower.

I wonder if @luciofranco has any ideas about this.

@rcoh maybe you can chime in here a bit?

I don't necessarily want to impose our retry needs on others, but as a data point here are things we need:

• Classifier interface that has access to the full Result<T, E> (and not just E)
• The ability for the retry service to modify the responses, eg. set metadata about the number of retries required, set a custom error if you ran out of retries, etc. I sketched a possible design for this in #546

Our retry policy behavior (around number of retries, backoff length, etc.) is far too complex to be well served by something shared, unfortunately, so I don't think we have much need for batteries included policies.

Those things are all possible using the existing retry trait, right? And, if they're not, that's an issue with the trait itself, not with any default policies, correct?

My goal with this PR was just to include some basic implementations that are sufficient for simple retry logic — of course, if there are more complex use cases, nothing prevents a user from implementing the trait themselves, but I'm not sure that that should mean that it's a bad idea to include some simpler default behavior.

@hdevalence sorry this has taken so long to land.

My opinion from 2 months still lands. If we fix the small docs things I would be okay with merging this. I think the discussion about whether or not to change Retry in a breaking way is a separate discussion.

Do you wanna drive this home or should I take over?

@davidpdrsn I think I've lost context on this one, so feel free to push it over the finish line.