Fix the behavior of OnHedging event
#1625
Conversation
martintmk
force-pushed
the
mtomka/fix-onhedging-event
branch
from
b4e97ef
to
e07cee5
Compare
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1625 +/- ##
==========================================
- Coverage 84.65% 84.63% -0.03%
==========================================
Files 306 306
Lines 6829 6819 -10
Branches 1045 1045
==========================================
- Hits 5781 5771 -10
Misses 839 839
Partials 209 209
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| /// <remarks> | ||
| /// This context is cloned from <see cref="PrimaryContext"/>. | ||
| /// </remarks> | ||
| public ResilienceContext ActionContext { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the zero-based hedging attempt number. |
There was a problem hiding this comment.
Is this the correct explanation? Looking at this fresh, if this is zero-based shouldn't we call it an index rather than a number?
There was a problem hiding this comment.
IF you don't mind i'll leave it as it is because we agreed on this in the API review and to me it sounds nicer that AttemptIndex.
There was a problem hiding this comment.
Ok, but I would expect the first value to be one if it's a number. I remember discussing calling it Number but I thought that was in the context of retries and it wasn't zero-based.
There was a problem hiding this comment.
For retries it's also called AttemptNumber and it's also zero-based. It's just the name we agreed on. Docs make it clear it starts from zero.
| /// <param name="primaryContext">The primary context received by hedging strategy.</param> | ||
| /// <param name="actionContext">The action context. cloned from the primary context.</param> |
There was a problem hiding this comment.
| /// <param name="primaryContext">The primary context received by hedging strategy.</param> | |
| /// <param name="actionContext">The action context. cloned from the primary context.</param> | |
| /// <param name="primaryContext">The primary context received by the hedging strategy.</param> | |
| /// <param name="actionContext">The action context cloned from the primary context.</param> |
| /// <remarks> | ||
| /// This context is cloned from <see cref="PrimaryContext"/>. | ||
| /// </remarks> | ||
| public ResilienceContext ActionContext { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the zero-based hedging attempt number. |
There was a problem hiding this comment.
Ok, but I would expect the first value to be one if it's a number. I remember discussing calling it Number but I thought that was in the context of retries and it wasn't zero-based.
Details on the issue fix or feature implementation
OnHedgingwas called even for last attempt, it caused that additional event to be reported.OnHedgingis called consistently just before the hedged task starts executing.OnHedgingArguments<T>OutcomeandDurationproperties as these are giving non-deterministic results based on what previous task finished first. If someone needs these we can add it later, however, with different API that allows deterministic results, for example access by attempt number.ActionContextproperty to be consistent withHedgingActionGeneratorArguments<T>Confirm the following