#1875 Use Polly v8 syntax #1914

RaynaldM · 2024-01-09T15:42:32Z

Closes #1875

Use Polly v8 syntax #1875

Proposed Changes

Use Polly 8's specific syntax to define resiliencePipelines (vs. PollyPolicies)

"Polly v8 introduces the concept of resilience pipelines, a powerful tool that blends one or more resilience strategies. This new API foundation echoes the Policy Wrap of previous versions but is now integrated seamlessly into the core API. All strategies start with a pipeline."

in v7, we written

// Create and use the IAsyncPolicy
IAsyncPolicy asyncPolicy = Policy
    .Handle<Exception>()
    .WaitAndRetryAsync(3, _ => TimeSpan.FromSeconds(1));

in v8, it becomes

// Use the ResiliencePipelineBuilder to start building the resilience pipeline
ResiliencePipeline pipeline = new ResiliencePipelineBuilder()
    .AddRetry(new RetryStrategyOptions
    {
        ShouldHandle = new PredicateBuilder().Handle<Exception>(),
        Delay = TimeSpan.FromSeconds(1),
        MaxRetryAttempts = 3,
        BackoffType = DelayBackoffType.Constant
    })
    .Build(); // After all necessary strategies are added, call Build() to create the pipeline.

…siliencePipeline

created tests for PollyResiliencePipelineDelegatingHandler created tests for PollyQoSResiliencePipelineProvider

…oviderTests

1875 use polly v8 syntax

…ensions` and `PollyQoSResiliencePipelineProvider` classes, as well as modifications to the `PollyQoSTests` class. In the `OcelotBuilderExtensions` class, the `GetDelegatingHandler` method is now used instead of `GetDelegatingHandlerV7` in the `AddPolly` methods. The `AddPolly` method without parameters has been simplified using a lambda expression. The `GetDelegatingHandler` method now returns a new instance of `PollyResiliencePipelineDelegatingHandler` instead of `PollyPoliciesDelegatingHandler`. In the `PollyQoSResiliencePipelineProvider` class, a comment has been added suggesting the use of `ResiliencePipelineRegistry<TKey>.GetOrAddPipeline` due to its thread-safe nature. The `MinimumThroughput` property in `circuitBreakerStrategyOptions` now uses the value from `route.QosOptions.ExceptionsAllowedBeforeBreaking`. In the `PollyQoSTests` class, the `QoSOptions` used in the test configurations have been updated. Two tests, `Should_open_circuit_breaker_then_close` and `Open_circuit_should_not_effect_different_route`, have been modified to repeat the same request as the minimum `ExceptionsAllowedBeforeBreaking` is now 2. The `GivenThereIsAPossiblyBrokenServiceRunningOn` method now delays for 2.1 seconds when the request count is 2, to ensure the circuit is open. Changes: 1. `OcelotBuilderExtensions` class updated to use `GetDelegatingHandler` method. 2. `AddPolly` methods in `OcelotBuilderExtensions` class simplified. 3. `GetDelegatingHandler` method in `OcelotBuilderExtensions` class now returns a new instance of `PollyResiliencePipelineDelegatingHandler`. 4. Comment added in `PollyQoSResiliencePipelineProvider` class suggesting the use of `ResiliencePipelineRegistry<TKey>.GetOrAddPipeline`. 5. `MinimumThroughput` property in `circuitBreakerStrategyOptions` updated to use value from `route.QosOptions.ExceptionsAllowedBeforeBreaking`. 6. `QoSOptions` in `PollyQoSTests` class updated. 7. `Should_open_circuit_breaker_then_close` and `Open_circuit_should_not_effect_different_route` tests in `PollyQoSTests` class updated. 8. `GivenThereIsAPossiblyBrokenServiceRunningOn` method in `PollyQoSTests` class updated to delay for 2.1 seconds when request count is 2.

fixed PollyQoSTests AcceptanceTests

raman-m · 2024-01-09T15:46:52Z

This branch is 9 commits ahead of, 3 commits behind develop.

…syntax

raman-m · 2024-01-10T11:04:49Z

Ray, build 3064 has failed!
Commit 8375908 looks like a bad merge 😉 Please fix the build!

…es in sample

…er`. This new implementation replaces the previous dictionary-based approach for managing cache for resilience pipelines.

Use `ResiliencePipelineRegistry` in `PollyQoSResiliencePipelineProvider`

raman-m · 2024-01-15T10:53:04Z

I love when Raynald develops features! 😍

raman-m · 2024-01-19T11:44:09Z

@RaynaldM I'm very sorry... But we have merge conflicts ❗ Please resolve them, and I'll review.

Update docs according to the Polly v8 implementations and QoS design upgrade

raman-m · 2024-03-08T16:20:13Z

@ggnaegi Please review and approve! I believe the PR is ready for delivery.
I've got necessary explanations by Ray, and I have a good clarity now.
Seems, nothing to improve.

ggnaegi · 2024-03-08T16:31:50Z

@ggnaegi Please review and approve! I believe the PR is ready for delivery.
I've got necessary explanations by Ray, and I have a good clarity now.
Seems, nothing to improve.

@raman-m Ok, but it's going to be late, around 11 your time.

ggnaegi

some minor changes, thanks!

src/Ocelot.Provider.Polly/v7/PollyPoliciesDelegatingHandler.cs

src/Ocelot.Provider.Polly/v7/PollyQoSProvider.cs

src/Ocelot.Provider.Polly/PollyQoSProviderBase.cs

src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs

src/Ocelot.Provider.Polly/PollyResiliencePipelineDelegatingHandler.cs

src/Ocelot.Provider.Polly/OcelotBuilderExtensions.cs

src/Ocelot.Provider.Polly/PollyQoSProviderBase.cs

raman-m · 2024-03-09T11:39:49Z

What I'm worrying now is increased duration of running unit tests by ~30s !
Locally it takes 27 seconds to run new 20 unit tests:

So, our CircleCI builds will take not 12m but 12m 30s ❗ Last build took 12m 45s!
Seems nothing to do with that. But in future we could move all package tests to separate testing project and release the package independently from Ocelot.nupkg

But acceptance tests cannot be moved unfortunately, and they will stay in the current testing project.

ggnaegi · 2024-03-09T11:42:04Z

@raman-m commented on Mar 9:

Seems nothing to do with that. But in future we could move all package tests to separate testing project and release the package independently from Ocelot.nupkg

Yes you're right!

ggnaegi · 2024-03-09T13:38:36Z

@raman-m commented on Mar 6

I don't know, probably some design issues since I agree with you about the HttpContextAccessor. Probably the way the object is instantiated.

raman-m · 2024-03-11T08:26:41Z

@ggnaegi commented on Mar 9

The problem is that your design of the delegate was reused in the new delegate class by Raynald.
So, seems it is a little bit late to fix design issue doing some refactoring. But we can skip it for now, and let's keep in mind that this design issue can be fixed in future PR...

ggnaegi · 2024-03-11T08:28:48Z

@ggnaegi commented on Mar 9

The problem is that your design of the delegate was reused in the new delegate class by Raynald.
So, seems it is a little bit late to fix design issue doing some refactoring. But we can skip it for now, and let's keep in mind that this design issue can be fixed in future PR...

@raman-m yes but I think it's quite deeply rooted in the Ocelot's design somewhere. You know, the kind of of classes that are not injected with DI.

raman-m · 2024-03-12T12:35:30Z

@ggnaegi commented on Mar 9

Great! We have the consensus! So, at least after the release I will create a TODO task to start brainstorming how to separate Polly Provider package to a separate Visual Studio solution with its own testing project. Possibly we could brainstorm how to adjust release process also.

TODO

Move Ocelot.Provider.Polly package to a separate solution

ggnaegi · 2024-03-12T20:33:46Z

@ggnaegi commented on Mar 9

Great! We have the consensus! So, at least after the release I will create a TODO task to start brainstorming how to separate Polly Provider package to a separate Visual Studio solution with its own testing project. Possibly we could brainstorm how to adjust release process also.

TODO

Move Ocelot.Provider.Polly package to a separate solution

@raman-m, I don't think that's the problem. The problem is more related with the fact that the QoS classes don't know the polly provider, since it's provided in a project that is not referenced in ocelot base. That's why I finally used this ugly hack.

raman-m · 2024-03-12T21:41:08Z

@ggnaegi Cherry pick 0ca4b18 once again?

raman-m · 2024-03-13T14:15:55Z

@ggnaegi I expect your fixing code review issues and approving finally till Friday to allow Raynald to merge the PR on Friday, March 15. 🆗 ❔

raman-m · 2024-03-14T09:19:49Z

@RaynaldM Ready for delivery! ✅

Code review ✔️✔️
Team approvals ✔️
Unit testing ✔️
Acceptance tests ❔ updated
Updated docs ✔️

Ray and others added 9 commits December 7, 2023 15:02

#1844 More open customization of Polly use

b223f7c

Merge branch '1844-more-open-customization-of-polly-use' into pollyRe…

547dcf8

…siliencePipeline

First step: implementation done, The UTs have yet to be determined

eeba4ff

reverted changes in PollyPoliciesDelegatingHandlerTests

33207a2

created tests for PollyResiliencePipelineDelegatingHandler created tests for PollyQoSResiliencePipelineProvider

renaming variables for better clarity in PollyQoSResiliencePipelinePr…

700396a

…oviderTests

Merge pull request #1907 from brikibsw/1875-use-polly-v8-syntax

5abf883

1875 use polly v8 syntax

Merge pull request #1909 from brikibsw/1875-use-polly-v8-syntax

95af855

fixed PollyQoSTests AcceptanceTests

end of implementation

bc259c3

RaynaldM linked an issue Jan 9, 2024 that may be closed by this pull request

Use Polly v8 syntax #1875

Closed

raman-m changed the title ~~1875 use polly v8 syntax~~ #1875 Use Polly v8 syntax Jan 9, 2024

Merge remote-tracking branch 'origin/develop' into 1875-use-polly-v8-…

8375908

…syntax

raman-m assigned RaynaldM Jan 10, 2024

raman-m added feature A new feature Jan'24 January 2024 release labels Jan 10, 2024

raman-m added this to the January'24 milestone Jan 10, 2024

RaynaldM and others added 4 commits January 10, 2024 15:27

remove "Microsoft.VisualStudio.Azure.Containers.Tools.Targets" packag…

87fba63

…es in sample

Use ResiliencePipelineRegistry in `PollyQoSResiliencePipelineProvid…

440cc3f

…er`. This new implementation replaces the previous dictionary-based approach for managing cache for resilience pipelines.

merge with upstream

a01d0c7

Merge pull request #1917 from brikibsw/1875-use-polly-v8-syntax

b9c9940

Use `ResiliencePipelineRegistry` in `PollyQoSResiliencePipelineProvider`

RaynaldM requested a review from raman-m January 16, 2024 09:46

RaynaldM added 5 commits January 25, 2024 16:53

merge with develop

25a6818

fix conflict

79018e9

merge with develop

ed75064

end of merge of develop (+polly package uptdate)

8abe29b

fix polly update

c20fccf

raman-m self-requested a review March 8, 2024 15:10

Update qualityofservice.rst

02a7d27

Update docs according to the Polly v8 implementations and QoS design upgrade

raman-m approved these changes Mar 8, 2024

View reviewed changes

ggnaegi requested changes Mar 8, 2024

View reviewed changes

raman-m added 2 commits March 9, 2024 12:50

More XML docs

c513b98

Make the base class abstract

a5bbabf

some code cleanup

f4aca4e

ggnaegi added 2 commits March 12, 2024 21:34

some comments

0ca4b18

remove comments

95e2bcf

ggnaegi approved these changes Mar 13, 2024

View reviewed changes

Re-use logger and fix warning

9bcad64

raman-m added 2 commits March 15, 2024 13:24

Merge branch 'develop' into 1875-use-polly-v8-syntax

136afa8

Final review of QoS docs

e033c80

RaynaldM merged commit d6eefc8 into develop Mar 15, 2024
2 checks passed

raman-m deleted the 1875-use-polly-v8-syntax branch March 15, 2024 12:52

This was referenced Mar 23, 2024

Retry calls to downstream #1093

Closed

Unexpected behavior of Polly #1153

Closed

Release 23.2 #2023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1875 Use Polly v8 syntax #1914

#1875 Use Polly v8 syntax #1914

RaynaldM commented Jan 9, 2024 •

edited by raman-m

raman-m commented Jan 9, 2024

raman-m commented Jan 10, 2024 •

edited

raman-m commented Jan 15, 2024

raman-m commented Jan 19, 2024

raman-m commented Mar 8, 2024 •

edited

ggnaegi commented Mar 8, 2024

ggnaegi left a comment

raman-m commented Mar 9, 2024 •

edited

ggnaegi commented Mar 9, 2024 •

edited by raman-m

ggnaegi commented Mar 9, 2024 •

edited by raman-m

raman-m commented Mar 11, 2024

ggnaegi commented Mar 11, 2024

raman-m commented Mar 12, 2024

ggnaegi commented Mar 12, 2024

TODO

raman-m commented Mar 12, 2024

raman-m commented Mar 13, 2024

raman-m commented Mar 14, 2024 •

edited

#1875 Use Polly v8 syntax #1914

#1875 Use Polly v8 syntax #1914

Conversation

RaynaldM commented Jan 9, 2024 • edited by raman-m

Closes #1875

Proposed Changes

raman-m commented Jan 9, 2024

raman-m commented Jan 10, 2024 • edited

raman-m commented Jan 15, 2024

raman-m commented Jan 19, 2024

raman-m commented Mar 8, 2024 • edited

ggnaegi commented Mar 8, 2024

ggnaegi left a comment

Choose a reason for hiding this comment

raman-m commented Mar 9, 2024 • edited

ggnaegi commented Mar 9, 2024 • edited by raman-m

ggnaegi commented Mar 9, 2024 • edited by raman-m

raman-m commented Mar 11, 2024

ggnaegi commented Mar 11, 2024

raman-m commented Mar 12, 2024

TODO

ggnaegi commented Mar 12, 2024

TODO

raman-m commented Mar 12, 2024

raman-m commented Mar 13, 2024

raman-m commented Mar 14, 2024 • edited

RaynaldM commented Jan 9, 2024 •

edited by raman-m

raman-m commented Jan 10, 2024 •

edited

raman-m commented Mar 8, 2024 •

edited

raman-m commented Mar 9, 2024 •

edited

ggnaegi commented Mar 9, 2024 •

edited by raman-m

ggnaegi commented Mar 9, 2024 •

edited by raman-m

raman-m commented Mar 14, 2024 •

edited