Skip to content

[Docs] Add clarification about property precedence #1918

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.

Sign up for GitHub

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 18 commits into from
Jan 24, 2024
Merged

[Docs] Add clarification about property precedence #1918

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f7b6a6c
Add clarification
peter-csala Jan 23, 2024
a79988d
Fix copy-paste issue
peter-csala Jan 23, 2024
106df37
Add description about constant backoff
peter-csala Jan 24, 2024
9345f5f
Fix spellcheck
peter-csala Jan 24, 2024
a1515c8
Apply suggestions from code review
peter-csala Jan 24, 2024
4607b93
Apply suggestions from code review
peter-csala Jan 24, 2024
aef2b88
Add description to each property
peter-csala Jan 24, 2024
eb4b475
Fix markdown lint issue
peter-csala Jan 24, 2024
d28bd3e
Apply suggestions from code review
peter-csala Jan 24, 2024
027c732
Fix markdown lint issue
peter-csala Jan 24, 2024
1e71491
Add description about linear backoff
peter-csala Jan 24, 2024
7ad36e0
Fix spellcheck issue
peter-csala Jan 24, 2024
4614dcb
Add description about exponential backoff
peter-csala Jan 24, 2024
dfbfb3d
Add examples
peter-csala Jan 24, 2024
62665fa
Apply suggestion
peter-csala Jan 24, 2024
8cf291a
Apply suggestion
peter-csala Jan 24, 2024
e39d09c
Correct null return value handling for DelayGenerator
peter-csala Jan 24, 2024
622cecc
Fix broken link
peter-csala Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions docs/strategies/retry.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,11 @@ If the `ShouldHandle` predicate returns `true` and the next attempt number is no

There are many properties that may contribute to this calculation:

- `Delay`
- `DelayGenerator`
- `MaxDelay`
- `UseJitter`
- `BackoffType`
- `BackoffType`: It specifies which calculation algorithm should run.
- `Delay`: If only this one is specified then it will be used as is. If others are defined as well then this will be used as a _base delay_.
- `DelayGenerator`: If specified then it will overwrite other properties based calculation. **Except** if it returns a negative `TimeSpan` where the other properties based calculation is used.
- `MaxDelay`: If specified then it caps the delay if the calculated one is greater than this value. **Except** if `DelayGenerator` is used where no capping is applied.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `MaxDelay`: If specified then it caps the delay if the calculated one is greater than this value. **Except** if `DelayGenerator` is used where no capping is applied.
- `MaxDelay`: If specified then it caps the delay if the calculated one is greater than this value. **Except** if `DelayGenerator` is used where no capping is applied.

Just wondering, shall we apply the capping to values produced by generator too?

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 would say yes. That was my intuition before I checked the implementation. But let's deal with that in a separate PR if you don't mind.

Copy link
Contributor

@martintmk martintmk Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My intuition says yes too, the less special cases, the better. The only downside is that sometimes the Retry-After won't be fully respected if the value is greater than MaxDelay.

https://github.com/dotnet/extensions/blob/9f25d9d74674483298cd167cf8ff6e9d9026bf20/src/Libraries/Microsoft.Extensions.Http.Resilience/Polly/HttpRetryStrategyOptions.cs#L57

Thoughts @martincostello

Copy link
Member

Choose a reason for hiding this comment

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

I would agree we should apply it - Retry-After isn't necessarily authoritative and the user may wish to try earlier by overriding. If we do that we should make it clear in the documentation.

Copy link
Contributor Author

@peter-csala peter-csala Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is a concision decision by the strategy configurator that (s)he sets the MaxDelay. If it is set then the strategy should cap the value regardless how it is calculated/retrieved IMHO.

- `UseJitter`: If enabled then it adds a random value between between -25% of `Delay` and +25% of `Delay`. **Except** if `BackoffType` is set to `Exponential` where jitter is always used.

> [!IMPORTANT]
> The summarized description below is an implementation detail. It may change in the future without notice.
Expand All @@ -137,7 +137,8 @@ Step 1: Calculating the base delay:

Step 2: Capping the delay if needed:

- If the previously calculated delay is greater than `MaxDelay` then `MaxDelay` will be used.
- If `MaxDelay` is not set then the previously calculated delay will be used.
- If `MaxDelay` is set and the previously calculated delay is greater than `MaxDelay` then `MaxDelay` will be used.

Step 3: Using the generator if supplied

Expand Down