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

Introduce ResilienceStrategyBuilder #1058

Merged
merged 12 commits into from
Mar 17, 2023

Conversation

martintmk
Copy link
Contributor

@martintmk martintmk commented Mar 16, 2023

The issue or feature being addressed

This PR adds a ResilienceStrategyBuilder class from v8-poc branch. It's used to build the instance of IResilienceStrategy.

Details on the issue fix or feature implementation

The following primitives are added:

  • ResilienceStrategyBuilder: the builder itself.
  • ResilienceStrategyBuilderContext: context that caries details for the strategy factory.
  • ResilienceStrategyBuilderOptions: the options of the builder. These will be epxanded later with more members such as clock, logging, telemetry.
  • ResilienceStrategyOptions: the options attached to individual strategy.

I'll add tests once we agree on the API surface.

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

Just a few things I spotted while reviewing the diff, but the API shape LGTM.

src/Polly.Core/Builder/IResilienceStrategyBuilder.cs Outdated Show resolved Hide resolved
src/Polly.Core/Builder/IResilienceStrategyBuilder.cs Outdated Show resolved Hide resolved
src/Polly.Core/Builder/ResilienceStrategyBuilder.cs Outdated Show resolved Hide resolved
src/Polly.Core/Builder/ResilienceStrategyBuilder.cs Outdated Show resolved Hide resolved
{
Validator.ValidateObject(Options, new ValidationContext(Options), true);

if (_entries.Count == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any benefit to special-casing a count of 1 to skip the loop and/or wrapping something that's just a pass-through?

src/Polly.Core/Builder/ResilienceStrategyBuilderContext.cs Outdated Show resolved Hide resolved
@martintmk martintmk force-pushed the mtomka/introduce-IResilienceStrategyBuilder branch from 6b3f6bc to d006124 Compare March 16, 2023 16:55
@martintmk martintmk force-pushed the mtomka/introduce-IResilienceStrategyBuilder branch from d006124 to 6f3f774 Compare March 16, 2023 16:56
@martintmk martintmk changed the title Introduce IResilienceStrategyBuilder Introduce ResilienceStrategyBuilder Mar 16, 2023
@martintmk martintmk marked this pull request as ready for review March 16, 2023 20:18
Copy link
Member

@joelhulen joelhulen left a comment

Choose a reason for hiding this comment

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

With all the changes you made this morning, this PR looks good to me.

@martintmk martintmk merged commit 1aeef57 into main Mar 17, 2023
@martincostello martincostello deleted the mtomka/introduce-IResilienceStrategyBuilder branch March 17, 2023 16:10
@martincostello martincostello modified the milestones: v7.2.4, v8.0.0 Mar 17, 2023
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.

4 participants