Initial push for adding orchestration reuse ID#258
Initial push for adding orchestration reuse ID#258RyanLettieri wants to merge 7 commits intomicrosoft:mainfrom
Conversation
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
jviau
left a comment
jviau
left a comment
There was a problem hiding this comment.
I think you may have already updating the protobuf submodule, but was there merge issues in that repo? Looks like all the entity work is gone.
| TaskName orchestratorName, | ||
| object? input = null, | ||
| StartOrchestrationOptions? options = null, | ||
| HashSet<string>? orchestrationIdReusePolicy = null, |
There was a problem hiding this comment.
Please add this to StartOrchestrationOptions instead of its own parameter. All non-essential parameters for affecting orchestration start should go there.
There was a problem hiding this comment.
Also, should this be an enum with the available behaviors as values in it?
| TaskName orchestratorName, | ||
| object? input = null, | ||
| StartOrchestrationOptions? options = null, | ||
| HashSet<string>? orchestrationIdReusePolicy = null, |
There was a problem hiding this comment.
"ReusePolicy" isn't that clear to me. Was the name InstanceIdConflictBehavior considered?
|
This will also address #183 |
There was a problem hiding this comment.
I suggest the following approach:
- Create a new enum in
Abstractions.csprojto represent the possible states for this feature. This is the type the customer will interact with. - Add this as an optional value to
StartOrchestrationOptionsandSubOrchestrationOptions - Add a configurable default value to
DurableTaskClientOptionsandDurableTaskWorkerOptions. This default value will be used when a customer does not explicitly provide it on the options from point 2. - Update clients and workers to consume this as appropriate. The gRPC client/worker implementation would convert the public facing enum to the gRPC enum internally.
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="../../src/Grpc/Grpc.csproj" /> |
There was a problem hiding this comment.
This project should not reference gRPC. It is meant to be implementation agnostic.
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
| /// <param name="OrchestrationIdReusePolicy">The orchestration reuse policy. This allows for the reuse of an instance ID | ||
| /// as well as the options for it.</param> | ||
| public record StartOrchestrationOptions(string? InstanceId = null, DateTimeOffset? StartAt = null, | ||
| Dictionary<List<OrchestrationOptions.OrchestrationRuntimeStatus>, OrchestrationOptions.InstanceIdReuseAction>? OrchestrationIdReusePolicy = null); |
There was a problem hiding this comment.
I see what you are trying to solve here: making it more granular on what the behavior is based on the state of the orchestration. Which I do think is worth considering.
I see the following problems with this approach:
- This information does not align with the protobuf spec
- It is a bit unwieldy for customers to need to specify this dictionary.
Instead, here is what I recommend:
- Lets first evaluate if this is needed? Or can we decide and document what the behavior of the 3 individual policies are based on the orchestration state? For example, we may consider applying conflict resolution for non-terminal orchestrations only.
- If not, then we can expand the protobuf and our enum to have more states that better clarify what happens. IE:
ErrorAlways,ErrorIfRunningeg.
The end result should be customers need to only provide a single enum to this options object.
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
| /// <summary> | ||
| /// Enum describing the runtime status of the orchestration. | ||
| /// </summary> | ||
| public enum OrchestrationRuntimeStatus |
There was a problem hiding this comment.
I think we will need to have type-forwarding from this type in the client package.
| /// <param name="OrchestrationIdReusePolicy">The orchestration reuse policy. This allows for the reuse of an instance ID | ||
| /// if the instance ID referenced is in any of the states supplied in this parameter.</param> | ||
| public record StartOrchestrationOptions(string? InstanceId = null, DateTimeOffset? StartAt = null, | ||
| OrchestrationRuntimeStatus[]? OrchestrationIdReusePolicy = null); |
There was a problem hiding this comment.
Can we wrap this in a type: OrchestrationIdReuse, so we can have helpers:
OrchestrationIdReuse.IfTerminal; // has all terminal statuses included.
OrchestrationIdReuse.Always; // has all statuses included.
OrchestrationIdReuse.IfStatus(params OrchestrationRuntimeStatus[] statuses);
OrchestrationIdReuse.IfStatus(IEnumerable<OrchestrationRuntimeStatus> statuses);
OrchestrationIdReuse.Never; // equal to default.| Version = orchestratorName.Version, | ||
| InstanceId = options?.InstanceId ?? Guid.NewGuid().ToString("N"), | ||
| Input = this.DataConverter.Serialize(input), | ||
| OrchestrationIdReusePolicy = { }, |
There was a problem hiding this comment.
You need to map this from options correct?
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="../Core/Client.csproj" /> | ||
| <ProjectReference Include="../../Grpc/Grpc.csproj" /> |
There was a problem hiding this comment.
this shouldn't be depending on gRPC
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="6.0.0" /> |
There was a problem hiding this comment.
What are these packages needed for?
2 participants
Related Issue: #255