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

add support for retrying 429 (too many requests) responses #45

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

evanharmon
Copy link
Contributor

@evanharmon evanharmon commented May 23, 2024

Motivation

Adds support for retrying requests when orb responds with a 429 status code. https://docs.withorb.com/reference/rate-limits

Checklist

  • If altering the functionality or API surface of this crate, this PR has a corresponding CHANGELOG.md entry.
    • The format is based on Keep a Changelog. Add your entries below the [Unreleased] section header.
  • There are no deviations from Orb's API.

@evanharmon evanharmon self-assigned this May 23, 2024
@evanharmon evanharmon marked this pull request as ready for review May 24, 2024 14:43
@evanharmon evanharmon requested review from arusahni and jubrad May 24, 2024 15:19
src/config.rs Outdated
Comment on lines 58 to 68
/// Retry requests with a successful response of 429 (too many requests).
struct Retry429;
impl RetryableStrategy for Retry429 {
fn handle(&self, res: &Result<Response, reqwest_middleware::Error>) -> Option<Retryable> {
match res {
// Retry if response status is 429
Ok(success) if success.status() == 429 => Some(Retryable::Transient),
// Otherwise do not retry a successful request
Ok(_) => None,
// Retry failures due to network errors
Err(error) => default_on_request_failure(error),
Copy link

Choose a reason for hiding this comment

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

based on the default strategy, I'm wondering if there are same Retryable::Fatal cases not being caught?
https://docs.rs/reqwest-retry/latest/src/reqwest_retry/retryable_strategy.rs.html#112

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't read the default strategy well enough. I've reverted to using the default strategy instead of specifying a new one. I see the default strategy handles the 429 status code and some other cases.

src/client.rs Outdated
@@ -39,7 +40,7 @@ pub mod taxes;
/// [`Arc`]: std::sync::Arc
#[derive(Debug)]
pub struct Client {
pub(crate) inner: reqwest::Client,
pub(crate) client_retryable: ClientWithMiddleware,
Copy link

Choose a reason for hiding this comment

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

I think you could keep this named inner? The reason this was changed in https://github.com/MaterializeInc/rust-frontegg/pull/13/files was because there was no true inner client, we selectively chose one of two clients depending on the http request method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK that makes sense. I've reverted back to using inner.

}
}
}

impl ClientBuilder {
/// Sets the policy for retrying failed API calls.
///
/// Note that the created [`Client`] will retry all API calls that return a 429 status code.
Copy link

Choose a reason for hiding this comment

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

If we're using the default policy I don't believe this is entirely accurate, it'll also retry server timeouts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants