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

[Docs] Fix the policywrap sample #1728

Merged

Conversation

peter-csala
Copy link
Contributor

Pull Request

The issue or feature being addressed

Details on the issue fix or feature implementation

  • V7's sample code created a policy chain where the timeout was the outer and the retry was the inner
  • Fixed the V7's sample code to have the same behavior as V8's sample
  • Also extended the comment to further clarify which one is the outer and which one is the inner

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

docs/migration-v8.md Outdated Show resolved Hide resolved
docs/migration-v8.md Outdated Show resolved Hide resolved
@martincostello martincostello enabled auto-merge (squash) October 25, 2023 10:10
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (00cd051) 84.56% compared to head (c47d833) 84.56%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1728   +/-   ##
=======================================
  Coverage   84.56%   84.56%           
=======================================
  Files         307      307           
  Lines        6790     6790           
  Branches     1043     1043           
=======================================
  Hits         5742     5742           
  Misses        839      839           
  Partials      209      209           
Flag Coverage Δ
linux 84.56% <ø> (ø)
macos 84.56% <ø> (ø)
windows 84.56% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@peter-csala
Copy link
Contributor Author

peter-csala commented Oct 25, 2023

@martintmk In case of V8 the FIFO statement is incorrect IMHO. The firstly added strategy will be executed last.

So, first you define the outer most and last the inner most.

Which is the same order as in case of V7 (first parameter is the outer most, last parameter is the inner most).

If you agree then I will update this section to reflect the current behavior.

Dotnet fiddle: https://dotnetfiddle.net/znmcPA

@peter-csala peter-csala force-pushed the fix-policywrap-migration-sample branch from 6340a65 to 4daee53 Compare October 29, 2023 13:26
@martincostello martincostello merged commit 94e23aa into App-vNext:main Oct 29, 2023
18 checks passed
@martintmk
Copy link
Contributor

@martintmk In case of V8 the FIFO statement is incorrect IMHO. The firstly added strategy will be executed last.

So, first you define the outer most and last the inner most.

Which is the same order as in case of V7 (first parameter is the outer most, last parameter is the inner most).

If you agree then I will update this section to reflect the current behavior.

Dotnet fiddle: https://dotnetfiddle.net/znmcPA

They problem is relying on the callback events, what's important is the when retry strategy starts executing and when it ends. Yes, it's true that the timeout event is reported first, but at that point the retry strategy is already executing.

If you put custom strategy that just reports events before and after the callback is executed, it would become clear.

@peter-csala
Copy link
Contributor Author

They problem is relying on the callback events, what's important is the when retry strategy starts executing and when it ends. Yes, it's true that the timeout event is reported first, but at that point the retry strategy is already executing.

If you put custom strategy that just reports events before and after the callback is executed, it would become clear.

Sorry @martintmk , but I'm a bit lost. Could you please rephrase your statement? (I'm not sure I understand it 100%).

@martintmk
Copy link
Contributor

They problem is relying on the callback events, what's important is the when retry strategy starts executing and when it ends. Yes, it's true that the timeout event is reported first, but at that point the retry strategy is already executing.
If you put custom strategy that just reports events before and after the callback is executed, it would become clear.

Sorry @martintmk , but I'm a bit lost. Could you please rephrase your statement? (I'm not sure I understand it 100%).

I believe this test conveys what I am trying to say. It adds multiple strategies to the pipeline and asserts the order of execution:

public void AddPipeline_Multiple_Ok()

@peter-csala
Copy link
Contributor Author

peter-csala commented Oct 30, 2023

ResiliencePipelineBuilderTests.cs

I understand that

  • in case of Before the order is from outer to inner and
  • in case of After the order is from inner to outer.

(sequence diagram)


I think I get it what do you want to describe.

Forgot the onRetry and onTimeout delegates and let's focus on this example:

var retryPolicy = Policy.Handle<Exception>().WaitAndRetryAsync(2, _ => TimeSpan.FromSeconds(0.5));
var timeoutPolicy = Policy.TimeoutAsync(TimeSpan.FromSeconds(1));

var wrappedPolicy = Policy.WrapAsync(retryPolicy, timeoutPolicy);

Console.WriteLine("Outer: " + wrappedPolicy.Outer.GetType().Name);
Console.WriteLine("Inner: " + wrappedPolicy.Inner.GetType().Name);

and the output:

Outer: AsyncRetryPolicy
Inner: AsyncTimeoutPolicy

are you convinced ?

@peter-csala peter-csala deleted the fix-policywrap-migration-sample branch October 31, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants