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

implement RetryPolicy for ExponentialBackoffTimed #16

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

bbaldino
Copy link
Contributor

@bbaldino bbaldino commented Jan 25, 2024

This PR seeks to implement the changes discussed in TrueLayer/reqwest-middleware#109. Mainly: that there is not currently a way to define a RetryPolicy which can observe a maximum retry duration. There did exist ExponentialBackoffTimed which, from what I can tell, sought to implement this, but since it did not implement RetryPolicy there was no way to pass it and have each new request have an independent start time. I believe this also obsolesces the need for ExponentialBackoffWithStart, so I removed it.

I did consider another approach:

  1. Rather than changing the should_retry API, we could keep the task start time in the RetryPolicy itself, but:
    • The soonest we'd be able to set the task start time would be in the first call to should_retry, which isn't really accurate
    • This makes the RetryPolicy implementation stateful, which I don't think makes sense considering only a single one is passed in.
    • We could change the policy passed in to be more of a policy "factory" or add additional methods to RetryPolicy (like request_started, but those are even bigger changes.

Definitely open to other suggestions, though I think the code here is consistent with what was discussed in the linked ticket. Will link the corresponding reqwest_middleware PR as well. corresponding reqwest-middleware PR.

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.

So this looks good to me.

For context, I believe I made the change this way to avoid a big breaking change in the primary API of the crate: the should_retry function.

In hindsight, that might not have been the best trade-off!

Thank you for the issue and PR 🙏

I've asked @tl-rodrigo-gryzinski to take a look too.

@bbaldino
Copy link
Contributor Author

@ThomWright we good to merge here? if so I can update the reqwest-middleware pr once we get a new retry-policies version released with this change

@eopb eopb merged commit 38dc48a into TrueLayer:main Mar 4, 2024
5 checks passed
@ThomWright
Copy link
Member

@bbaldino Ethan has kindly merged and published the changes.

Please feel free to make the changes in request-middleware and we'll take a look 🙂

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.

4 participants