search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.24k stars 1.62k forks source link

#2085 Enforce Polly v8 Circuit Breaker as the primary strategy #2086

Closed RaynaldM closed 1 month ago

RaynaldM commented 1 month ago

Fixes #2085

Discussion

Proposed Changes

raman-m commented 1 month ago

Ray, thank you for your feedback in the related discussion! For future reference, could you please create a feature branch in your personal folder on the origin? Use this format: RaynaldM/2083.

Additionally, while having two unit tests is great, we also require at least one acceptance test that verifies the user scenario and the hotfix. Your contribution of an acceptance test would be greatly appreciated.

RaynaldM commented 1 month ago

Ray, thank you for your feedback in the related discussion! For future reference, could you please create a feature branch in your personal folder on the origin? Use this format: RaynaldM/2083.

Additionally, while having two unit tests is great, we also require at least one acceptance test that verifies the user scenario and the hotfix. Your contribution of an acceptance test would be greatly appreciated.

Sorry for branch issue, I did it quickly... I'm not the more qualified for UT, I will work on it later

raman-m commented 1 month ago

Let me check what acceptance tests do we have now...

raman-m commented 1 month ago

I have no good clarity on this https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs#L36 What does the condition options.TimeoutValue is int.MaxValue check actually? Why max value? Based on other line 52 https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs#L52 I expect that line 36 should check existence of the option, right? So, I expect this check 👉 

// this means the option does NOT exist
options.TimeoutValue == int.MaxValue || options.TimeoutValue == 0

Am I right, @RaynaldM ?

raman-m commented 1 month ago

Our official acceptance testing for Polly provider: https://github.com/ThreeMammals/Ocelot/blob/a034e8c1e3fc23a086ad10000c85615b9696a43e/test/Ocelot.AcceptanceTests/PollyQoSTests.cs#L9 Reviewing...