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 ResilienceStrategyRegistry #1085

Merged
merged 4 commits into from
Mar 28, 2023
Merged

Conversation

martintmk
Copy link
Contributor

@martintmk martintmk commented Mar 27, 2023

The issue or feature being addressed

#1084

Details on the issue fix or feature implementation

Registry allows registration and retrieval of strategies:

var registry = new ResilienceStrategyRegistry<string>();
var strategy = new ResilienceStrategyBuilder().AddTimeout(TimeSpan.FromSeconds(10));

registry.TryAdd("timeout-strategy", strategy);
registry.Get("timeout-strategy");

Or builders:

var registry = new ResilienceStrategyRegistry<string>();
registry.AddBuilder("timeout-strategy", (key, builder) => builder.AddTimeout(TimeSpan.FromSeconds(10));
registry.Get("timeout-strategy"); // strategy is dynamically created on-demand

The more advanced scenarios allow multi-instance support where one builder is repeatedly used to create multiple strategies. Registry then caches each of these strategies individually. We need this functionality to allow having distinct circuit breaker instances that are cached by composite keys.

Typical scenario for this is having HttpClient that sends requests to a different server across multiple geo-locations. We want to have a different circuit breaker instances assigned.

var registry = new ResilienceStrategyRegistry<StrategyId>(new ResilienceStrategyRegistryOptions<StrategyId>
{
    StrategyComparer = StrategyId.Comparer,
    BuilderComparer = StrategyId.BuilderComparer
});

registry.TryAddBuilder(StrategyId.Create("my-pipeline"), (id, builder) => builder.AddCircuitBreaker());

// access the strategy
registry.Get(StrategyId.Create("my-pipeline", "instance-A"));
registry.Get(StrategyId.Create("my-pipeline", "instance-B"));

In the follow-up (#1087) I would like to introduce Polly.Hosting or Polly.DependencyInjection project that integrates the ResilienceStrategyRegistry into the DI and allows defining the strategies at startup using IServiceCollection. That way, the core infra will be done and we can focus fully on strategies.

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

@martintmk martintmk added the v8 Issues related to the new version 8 of the Polly library. label Mar 27, 2023
@martintmk martintmk added this to the v8.0.0 milestone Mar 27, 2023
@martintmk martintmk self-assigned this Mar 27, 2023

namespace Polly.Core.Tests.Registry;

public record StrategyId(Type Type, string BuilderName, string InstanceName = "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geeknoid We can consider moving this to the core API as we would propably have to copy-paste this piece to our codebase. This struct allows:

  • Isolation of strategies by the type of the result they are handling.
  • Multi-instance support.

cc @martincostello

@martintmk martintmk changed the title Introduce ResilienceStrategyregistry Introduce ResilienceStrategyRegistry Mar 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2023

Codecov Report

Merging #1085 (5090a18) into main (94b309d) will increase coverage by 0.20%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
+ Coverage   74.83%   75.04%   +0.20%     
==========================================
  Files         170      173       +3     
  Lines        4208     4243      +35     
  Branches      787      790       +3     
==========================================
+ Hits         3149     3184      +35     
  Misses        854      854              
  Partials      205      205              
Flag Coverage Δ
linux ?
macos 75.04% <100.00%> (+0.20%) ⬆️
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../Polly.Core/Registry/ResilienceStrategyProvider.cs 100.00% <100.00%> (ø)
.../Polly.Core/Registry/ResilienceStrategyRegistry.cs 100.00% <100.00%> (ø)
...Core/Registry/ResilienceStrategyRegistryOptions.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

/// An options class used by <see cref="ResilienceStrategyRegistry{TKey}"/>.
/// </summary>
/// <typeparam name="TKey">The type of the key used by the registry.</typeparam>
public class ResilienceStrategyRegistryOptions<TKey>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document all the default values of the properties and what they do.

/// </summary>
/// <typeparam name="TKey">The type of the key.</typeparam>
/// <remarks>
/// The ResilienceStrategyRegistry class provides a way to organize and manage multiple resilience strategies
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
/// The ResilienceStrategyRegistry class provides a way to organize and manage multiple resilience strategies
/// This class provides a way to organize and manage multiple resilience strategies

@martintmk martintmk merged commit 2f84eab into main Mar 28, 2023
@martintmk martintmk deleted the mtomka/strategy-registry branch March 28, 2023 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 Issues related to the new version 8 of the Polly library.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants