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

feat: Allow total duration with max retries #15

Merged
merged 17 commits into from
Oct 11, 2023

Conversation

tl-marco-tormento
Copy link
Contributor

@tl-marco-tormento tl-marco-tormento commented Oct 3, 2023

This PR fixes one main problem, which is the fact that using a total duration we are not able to set a limit to the number of retries that will be made inside that timeframe.

We added a new build_with_total_retry_duration_and_max_retries method to ExponentialBackoffBuilder that calculates the number of retries that should be made if jitter 1.0 was applied (which means no jitter).
Since jitter can be 0..1, the calculated number of retries is effectively the max number of retries that can be made during that duration.

In should_retry we now check if either total duration or max attempts are exceeded.

We also made the exponential's base configurable: this will allow users to exponentially delay the retries more from one another.
The default remains 2, so this isn't a breaking change.

@tl-marco-tormento tl-marco-tormento requested a review from a team as a code owner October 3, 2023 08:58
Copy link
Member

@ThomWright ThomWright left a comment

Choose a reason for hiding this comment

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

I'm wondering if there's a more general-purpose API design here.

I'm thinking about:

  1. What if we want to set these independently?
  2. What if we want to calculate duration from max retries? Is that a sensible option?

For 1, what do you think about an API like:

let exponential_backoff_timed = ExponentialBackoff::builder()
    .retry_bounds(Duration::from_secs(1), Duration::from_secs(6 * 60 * 60))
    .max_retries(10)
    .build_with_total_retry_duration(Duration::from_secs(24 * 60 * 60));

or:

ExponentialBackoff::builder()
    .build_with_max_retries(5)
    .with_total_retry_duration(Duration::from_secs(24 * 60 * 60));

Not sure about 2 yet, but something like this:

image

I might be overcomplicating it, but keen to try to find a good general-purpose API which can cover these cases, instead of committing to specific-purpose APIs.

src/policies/exponential_backoff.rs Outdated Show resolved Hide resolved
src/policies/exponential_backoff.rs Show resolved Hide resolved
@tl-marco-tormento
Copy link
Contributor Author

tl-marco-tormento commented Oct 3, 2023

I'm thinking about:

  1. What if we want to set these independently?

What if we provide this API:

fn build_with_total_retry_duration(
        self,
        total_duration: Duration,
        max_retries: MaxRetries
    ) -> ExponentialBackoffTimed;

enum MaxRetries {
       Unbounded,
       Default,
       Fixed(u16)
}

User could:

  • select total_duration and MaxRetries::Unbounded => do any number of retries inside total_duration
  • select total_duration and MaxRetries::Default => we calculate max number of retries using total_duration and enforce whatever comes first
  • select total_duration and MaxRetries::Fixed(max_retries) => we enforce whatever comes first, max_retries is capped at the number we calculate using total_duration anyway
  1. What if we want to calculate duration from max retries? Is that a sensible option?

I think that should be the default when using the build_with_max_retries API, because I can't think of a case where I would want to set max_retries without capping the total_duration... do you have any example where that would be sensible?

@ThomWright
Copy link
Member

Is this a good illustration of how it will work?

image

What if we provide this API:

I like it. I think we might want to either find a better name for Default, or describe it really well in the doc comments.

A downside is that it's a breaking change?

I can't think of a case where I would want to set max_retries without capping the total_duration... do you have any example where that would be sensible?

If you constrain both the min and max intervals, you get a fairly good idea of how long something could take... but only as long as the task is short, which is maybe a key oversight!

I'm coming around to this idea.

@tl-marco-tormento
Copy link
Contributor Author

tl-marco-tormento commented Oct 3, 2023

Is this a good illustration of how it will work?

Yes, that's how the MaxRetries::Fixed(max_retries) would work.

I think we might want to either find a better name for Default

Big issues with naming here...

A downside is that it's a breaking change?

It is, we would deprecate the current build_with_max_retries and build_with_total_duration methods and provide the new API.

If you are onboard I'll start the implementation first thing tomorrow morning.

src/policies/exponential_backoff.rs Outdated Show resolved Hide resolved
src/policies/exponential_backoff.rs Outdated Show resolved Hide resolved
eopb
eopb previously approved these changes Oct 5, 2023
eopb
eopb previously approved these changes Oct 6, 2023
ThomWright
ThomWright previously approved these changes Oct 6, 2023
src/policies/exponential_backoff.rs Outdated Show resolved Hide resolved
@tl-marco-tormento tl-marco-tormento dismissed stale reviews from ThomWright and eopb via 6175bbd October 9, 2023 08:22
@tl-marco-tormento tl-marco-tormento merged commit 49df300 into main Oct 11, 2023
5 checks passed
@tl-marco-tormento tl-marco-tormento deleted the maps-570-allow-max-duration-with-max-retries branch October 11, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants