[WIP-DO NOT MERGE] Chaos and Fault Injection polices #553
Conversation
reisenberger
commented
Dec 30, 2018
•
reisenberger
commented
- per PR [WIP-DO NOT MERGE] Add InjectFault and InjectLatency features #508 to SHA f2bec42 EDIT: largely representing work by @mebjas and @vany0114
- plus v7.0.0-compatibility modifications, and other changes, by @reisenberger
+ per PR 508 to SHA f2bec42 + plus v7.0.0-compatibility modifications, and other changes, by @reisenberger
| /// <returns>The policy instance.</returns> | ||
| public static AsyncInjectBehaviourPolicy InjectBehaviourAsync( | ||
| Func<Task> behaviour, | ||
| Double injectionRate, |
There was a problem hiding this comment.
should we have a check for < 0 value? across the code base
There was a problem hiding this comment.
should we have a check for < 0 value? across the code base
Yes, this would be good. I saw the opportunity to add some quick boundary checks in the execution-phase in my contribution, these target the cases where the dynamic Func<..., double> variants generate some out-of-range double as well. But for cases where the injection rate is configured as a non-dynamic Double injectionRate, configuration-time checking would be an improvement.
Tagging this mentally to be done after we've moved the codebase over to the new repo.
There was a problem hiding this comment.
Don't know if I misunderstood, but I was taking a look at that, and I think the validation you put on ShouldInject method, cover all scenarios since the syntax classes are ensuring that at the end of the day the injectionRate will be always a lambda.
| Task<Exception> FaultLambda(Context _, CancellationToken __) => Task.FromResult<Exception>(fault); | ||
| Task<double> InjectionRateLambda(Context _) => Task.FromResult<Double>(injectionRate); | ||
|
|
||
| return new AsyncInjectOutcomePolicy<TResult>((Func<Context, CancellationToken, Task<Exception>>)FaultLambda, InjectionRateLambda, enabled); |
There was a problem hiding this comment.
why explicit recasting here?
Good q. Without it the compiler was giving an ambiguous overload resolution error.
Remove code erroneously left in by mistake
| { | ||
| if (enabled == null) throw new ArgumentNullException(nameof(enabled)); | ||
|
|
||
| Action<Context> latencyProvider = _ => { SystemClock.Sleep(latency, CancellationToken.None); }; |
There was a problem hiding this comment.
Action latencyProvider = _ => { SystemClock.Sleep(latency, CancellationToken.None); };
This guy is not being used, I can remove it after we've moved the codebase over to the new repo.
| /// <returns>The policy instance.</returns> | ||
| public static AsyncInjectBehaviourPolicy InjectBehaviourAsync( | ||
| Func<Task> behaviour, | ||
| Double injectionRate, |
There was a problem hiding this comment.
Don't know if I misunderstood, but I was taking a look at that, and I think the validation you put on ShouldInject method, cover all scenarios since the syntax classes are ensuring that at the end of the day the injectionRate will be always a lambda.
Bring Simmy codebase across from App-vNext/Polly#553 (and originally App-vNext/Polly#508). Represents original contributions by @vany0114 , @mebjas and @reisenberger, which @vany0114 has brought to the Simmy repo from Polly. Noting that all contributors @vany0114 , @mebjas and @reisenberger had signed the .NET Foundation Contributors License Agreement against the Polly repo.
|
Hi @vany0114 Re:
That is correct. It would be a small improvement for parameter validation to throw at configuration time rather than execution time for the constant cases, but not essential. Noted on the Simmy repo: Polly-Contrib/Simmy#15 |
|
Locking this thread (in preference to closing it right now, which reduces visibility), because this no longer represents the latest version of the codebase for this feature. Interested parties should head on over to the Simmy repo where this functionality is being finalised! |
|
Closing as this codebase now exists definitively in the Simmy repo, the readme now links out to Simmy and #499 still tracks on the issues page. |
