Ocelot
Ocelot copied to clipboard
ThreeMammals
#1875 Use Polly v8 syntax
Closes #1875
- #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.
Ray, build 3064 has failed! Commit https://github.com/ThreeMammals/Ocelot/pull/1914/commits/837590841f3aaf7311336ff6c9787607b01a5eb8 looks like a bad merge 😉 Please fix the build!
@RaynaldM I'm very sorry... But we have merge conflicts ❗ Please resolve them, and I'll review.
What more can I do? it works locally
Yes, Sometimes we have a surprise in CircleCI Docker environment. So, having green tests locally doesn't mean the tests will be green in CircleCI build.
Ray, It is better to rebase feature branches after each release PRs commits delivery to develop. So develop branch has "clean" starting point as merge commit d54836d51f6ea29a2013967e3649a23e19db28b3. Is it possible to rebase this feature branch? I see a lot of PRs merge commits and conflicts resolution... also some Files Changed are highlighted as fake diff. Or will we leave this branch as is?
@RaynaldM If locally in IDE it works and tests are green, but they are red in CircleCI build then please keep in mind that timeouts on CircleCI should be greater (longer). Try to increase timeout values. CircleCI env is quite unstable, sometimes it can be slow.
@ggnaegi Please help Ray, we need to intensify development of this release, because it is current! 😄
https://github.com/ThreeMammals/Ocelot/pull/1914/commits/1c30becfc02e9006e2013baf7908ebebd34aa41c Wow! Timings being increased in 10 times helped to fix tests in CircleCI env! 😂
- await Task.Delay(600);
+ await Task.Delay(6000);
But Why in 10 times? Maybe increasing in 2-3 times should help too?!.. LoL! Tears in my eyes :yum:
@RaynaldM 10 times are too much! This is 6 seconds running test!
Are you going to fix or not? Do you need a help in conflicts resolution?
P.S.
Pay your attention to the fact we have not much time for this release. I want to release on Friday, March 1. But sooner is better.
Look at the build 3540: its running time was increased to 13m 20s. But the running time in develop takes no more 12m ! So, by the tests of this PR we've added extra 1m 20s!!! So, this is too much! We have to review the tests trying to decrease running time.
Update
The next build 3542 took 12m 40s. So, we have extra 40-45 seconds, at least
Latest merge commits auto-removed actual version of OcelotBuilderExtensions !!! I am shocked! 🤯 All XML docs were lost! That's why it is better to rebase the branches after each release. But this feature branch looks ugly...
TODO
- [x] Re-add lost XML docs from commit https://github.com/ThreeMammals/Ocelot/commit/0404c2473d2f1b51849fcedb1512b8a86012b867; ✔️ commit https://github.com/ThreeMammals/Ocelot/pull/1914/commits/5ebb165e2e866153eedf8da993c9a188e413cea3
- [x] Deep code review in VS; ✔️ commit https://github.com/ThreeMammals/Ocelot/pull/1914/commits/e2306ace2429fb5e609ecaa1357954d0f7687249
@ggnaegi
What is that? 🤯
https://github.com/ThreeMammals/Ocelot/blob/5a3c55e358afb7853819e3370daad9bb15452dd9/src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs#L30
You've injected IHttpContextAccessor object as the _contextAccessor private member and then you get another service object from service collection?
https://github.com/ThreeMammals/Ocelot/blob/5a3c55e358afb7853819e3370daad9bb15452dd9/src/Ocelot.Provider.Polly/PollyPoliciesDelegatingHandler.cs#L17-L25
What was a problem to inject IPollyQoSProvider<HttpResponseMessage> object into the constructor?
Was some design issue to inject IPollyQoSProvider<HttpResponseMessage> directly?
I just cannot get it?! 😮
I expect the design with the constructor like this
public PollyPoliciesDelegatingHandler(
DownstreamRoute route,
IPollyQoSProvider<HttpResponseMessage> provider, // Our lovely provider from DI
IOcelotLoggerFactory loggerFactory)
{
_route = route;
_provider = provider; // !!! Injecting what we want directly
_logger = loggerFactory.CreateLogger<PollyPoliciesDelegatingHandler>();
}
and further code:
protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
var policy = _provider.GetPollyPolicyWrapper(_route).AsyncPollyPolicy; // !!! _provider is private readonly member
return await policy.ExecuteAsync(async () => await base.SendAsync(request, cancellationToken));
}
@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 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.
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.
@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!
@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.
@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 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.
@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.Pollypackage to a separate solution