Skip to content

Refactor the API for retry options #1067

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

Merged
merged 2 commits into from
Sep 2, 2022
Merged

Conversation

rylev
Copy link
Contributor

@rylev rylev commented Sep 2, 2022

This changes the API for specifying retry options to be more customizable while also being more future proof.

// Exponential retry with 10 total retries and an initial delay of 1 second
 let options = ClientOptions::default().retry(RetryOptions::exponential(
            ExponentialRetryOptions::default()
                .max_retries(10u32)
                .initial_delay(Duration::from_secs(1)),
        ));

// No retry
let options = ClientOptions::default()
     .retry(RetryOptions::none());

Note: the default remains an exponential strategy which should be appropriate for a vast majority of users. Therefore, these APIs will likely only be used by users with very specific needs.

Instead of RetryOptions being a transparent struct whose fields the user sets directly (thus making it impossible to extend in the future in a backwards compatible way), the options are now set through methods (e.g., RetryOptions::exponential, RetryOptions::fixed, etc.).

Each of these methods takes different set of arguements that are tailored just to the retry strategy at hand. Instead of requiring that the user be confronted with options that might not be relevant to their strategy of choice (e.g., the max_delay field had no meaning if the user was using a fixed retry strategy), the options are constrained just to the options that are relevant.

This does make instantiation ever so slightly more verbose as you cannot set options directly on the RetryOptions. Again, this is a feature not a bug since the options you want to set depend on the overall strategy you choose.

Lastly, this adds an additional retry strategy of custom that allows users to feed in their own Arc<dyn RetryPolicy>.

Closes #1049

@rylev rylev requested a review from bmc-msft September 2, 2022 08:51
@bmc-msft
Copy link
Contributor

bmc-msft commented Sep 2, 2022

This is a positive step forwards.

It might be beneficial for usability reasons to create From<ExponentialRetryOptions> for RetryOptions (and the like for others).

@bmc-msft bmc-msft merged commit 35a6d96 into Azure:main Sep 2, 2022
@rylev rylev deleted the refactored-retry branch September 2, 2022 14:31
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.

There is currently no way to directly set the retry_policy on a Pipeline.
2 participants