-
Notifications
You must be signed in to change notification settings - Fork 328
Add per-attempt retry metadata tags in OrchestrationContext.ScheduleWithRetry #1367
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
base: main
Are you sure you want to change the base?
Changes from all commits
fc0c0e5
662556d
6e5bb94
deb466e
9ac2f61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ namespace DurableTask.Core | |
| { | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Globalization; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Castle.DynamicProxy; | ||
|
|
@@ -204,10 +205,31 @@ public virtual Task<T> ScheduleWithRetry<T>(Type taskActivityType, RetryOptions | |
| /// <param name="retryOptions">Retry policy</param> | ||
| /// <param name="parameters">Parameters for the TaskActivity.Execute method</param> | ||
| /// <returns>Task that represents the execution of the specified TaskActivity</returns> | ||
| /// <remarks> | ||
| /// Per-attempt retry metadata is injected into the scheduled task's <see cref="ScheduleTaskOptions.Tags"/> | ||
| /// under the reserved <c>dt.retry.*</c> tag namespace (<c>dt.retry.attempt</c>, <c>dt.retry.maxAttempts</c>). | ||
| /// These tags flow through <c>ScheduleTaskOrchestratorAction.Tags</c> into the persisted | ||
| /// <c>TaskScheduledEvent.Tags</c>, where consumers (Functions extension, DTS Dashboard) can read them. | ||
| /// Backends that do not roundtrip <c>TaskScheduledEvent.Tags</c> | ||
| /// simply drop the tags at persistence; consumers fall back to a "metadata unavailable" path with no error. | ||
| /// The attempt counter is closure-local, so each call site builds its own counter; deterministic replay | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just double-checking, I'm assuming we confirmed this is actually true in testing (like a "live test", I see that we did test it in the unit tests)? "deterministic replay reproduces the same per-attempt values because invokes the closure the same number of times in the same order on replay." |
||
| /// reproduces the same per-attempt values because <see cref="RetryInterceptor{T}.Invoke"/> invokes the | ||
| /// closure the same number of times in the same order on replay. | ||
| /// </remarks> | ||
| public virtual Task<T> ScheduleWithRetry<T>(string name, string version, RetryOptions retryOptions, | ||
| params object[] parameters) | ||
| { | ||
| Task<T> RetryCall() => ScheduleTask<T>(name, version, parameters); | ||
| int attempt = 0; | ||
| Task<T> RetryCall() | ||
| { | ||
| attempt++; | ||
| ScheduleTaskOptions options = ScheduleTaskOptions.CreateBuilder() | ||
| .AddTag(RetryTags.Attempt, attempt.ToString(CultureInfo.InvariantCulture)) | ||
| .AddTag(RetryTags.MaxAttempts, retryOptions.MaxNumberOfAttempts.ToString(CultureInfo.InvariantCulture)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if |
||
| .Build(); | ||
| return ScheduleTask<T>(name, version, options, parameters); | ||
| } | ||
|
|
||
| var retryInterceptor = new RetryInterceptor<T>(this, retryOptions, RetryCall); | ||
| return retryInterceptor.Invoke(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // ---------------------------------------------------------------------------------- | ||
| // Copyright Microsoft Corporation | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // ---------------------------------------------------------------------------------- | ||
|
|
||
| namespace DurableTask.Core | ||
| { | ||
| /// <summary> | ||
| /// Reserved tag keys written by DTFx into <see cref="ScheduleTaskOptions.Tags"/> (and from there into | ||
| /// <see cref="History.TaskScheduledEvent.Tags"/>) when an activity is scheduled under a retry policy. | ||
| /// </summary> | ||
|
Comment on lines
+16
to
+19
|
||
| /// <remarks> | ||
| /// The <c>dt.</c> prefix is reserved for DTFx-injected metadata; customers must not use it on | ||
| /// caller-supplied tags. | ||
| /// </remarks> | ||
| internal static class RetryTags | ||
|
bachuv marked this conversation as resolved.
|
||
| { | ||
| /// <summary> | ||
| /// 1-based attempt counter for the current schedule attempt. Decimal string in | ||
| /// <see cref="System.Globalization.CultureInfo.InvariantCulture"/>. | ||
| /// </summary> | ||
| public const string Attempt = "dt.retry.attempt"; | ||
|
|
||
| /// <summary> | ||
| /// Policy ceiling — the <see cref="RetryOptions.MaxNumberOfAttempts"/> value from the | ||
| /// retry policy in effect for this schedule. Decimal string in | ||
| /// <see cref="System.Globalization.CultureInfo.InvariantCulture"/>. | ||
| /// </summary> | ||
| public const string MaxAttempts = "dt.retry.maxAttempts"; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this part of the comment?
I feel like it's leaking implementation details/we probably shouldn't be referencing Functions and DTS from here