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

[Feature request]: allow MaxRetryAttempts=0 #2297

Open
verdie-g opened this issue Sep 19, 2024 · 3 comments
Open

[Feature request]: allow MaxRetryAttempts=0 #2297

verdie-g opened this issue Sep 19, 2024 · 3 comments

Comments

@verdie-g
Copy link

verdie-g commented Sep 19, 2024

Is your feature request related to a specific problem? Or an existing feature?

I'm migrating from policies to resilience pipelines and I saw many of my tests failing because of

System.ComponentModel.DataAnnotations.ValidationException : The 'RetryStrategyOptions<HttpResponseMessage>' are invalid.
  Validation Errors:
    The field MaxRetryAttempts must be between 1 and 2147483647.

It seems like 0 is not accepted anymore.

The work around is to do

var pipeline = new ResiliencePipelineBuilder();
if (retryCount > 0)
{
    pipeline.AddRetry(new RetryStrategyOptions
    {
          ShouldHandle = new PredicateBuilder().Handle<Exception>(),
          MaxRetryAttempts = retryCount,
    });
}
pipeline.Build();

but it's kind of easy to miss.

In my case, the MaxRetryAttempts can come from dynamic configurations. So, changing a config to disable the pipeline by setting 0 to MaxRetryAttemps could actually break the application.

Describe the solution you'd like

When MaxRetryAttemps is set to 0, it would simply disable the retry.

Additional context

No response

@martincostello
Copy link
Member

I would argue allowing this could have the opposite effect of allowing you to accidentally configure a non-functional retry policy while still paying for the performance overhead of a retry that will never actually retry.

I think the change for v8 makes more overall sense (retrying 0 times isn't a retry), so the correct thing to do is your "workaround" of not adding a policy that doesn't make any sense in the first place.

Either way has an advantage and a disadvantage, and I think making mis-configuration apparent/obvious is better than silent failure (i.e. no retries).

@verdie-g
Copy link
Author

verdie-g commented Sep 19, 2024

Yeah I'm fine with that idea. Just wanted to open the discussion because I'm pretty sure we'll have an incident in the future where a retry config is set to 0.

@martincostello
Copy link
Member

I'm glad you test your policies though 😄

I've always maintained people should configure their policies and then test they behave the way they think they've configured them is correct.

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

No branches or pull requests

2 participants