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

build_with_total_retry_duration examples? #109

Open
bbaldino opened this issue Oct 4, 2023 · 4 comments
Open

build_with_total_retry_duration examples? #109

bbaldino opened this issue Oct 4, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@bbaldino
Copy link
Contributor

bbaldino commented Oct 4, 2023

Motivations

I know there have been a couple issues filed about confusion regarding ExponentialBackoffTimed--I had the same confusion myself, which is how I came across those issues. I think I understand the intent of the fix that made this change, but I'm wondering what an intended use of this API would look like (I searched around github, but couldn't find any examples of how others are using it).

Mainly: it feels awkward to have to create the policy each time a request is made. To me the appeal of middleware was being able to bake this sort of retry into the client such that code that just wants to make the request wouldn't have to worry about it...but the way this feature is set up it looks like calling code would need to create its own ClientWithMiddleware each time it creates a request to be able to pass for_task_started_at to the retry policy each time? Or is there another pattern that I'm missing?

Solution

It would be great to see some examples in the README of the intended usage of this API

Alternatives

n/a

Additional context

n/a

@bbaldino bbaldino added the enhancement New feature or request label Oct 4, 2023
@bbaldino
Copy link
Contributor Author

bbaldino commented Oct 4, 2023

I was also wondering: it looks like RetryTransientMiddleware has state to track the number of past retries, and RetryPolicy has specific support for the number of past retries...could RetryPolicy also take in the task start time and RetryTransientMiddleware track the task start time to enable ExponentialBackoffTimed to be able to implement RetryPolicy directly like ExponentialBackoff does?

If that's reasonable I could take a shot at implementing it.

@tl-rodrigo-gryzinski
Copy link
Contributor

Hey! Sorry for the delay responding. You're right, RetryTransientMiddleware should keep track of the task start time, I shouldn't have closed these other issues.

I think adding the task start time to RetryPolicy::should_retry is a reasonable change, if you're still up for implementing it we're open for PRs 🙇

@bbaldino
Copy link
Contributor Author

bbaldino commented Jan 9, 2024

Thanks...we ended up working around this so haven't looked at it in a while, but if I can find some time I'll come back and take a shot at it.

@bbaldino
Copy link
Contributor Author

I opened TrueLayer/retry-policies#16 and #113 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

2 participants