Open cgillum opened 1 year ago
My 2c
public class ScheduleNewWorkflowOptions
{
// start time
public ReusePolicy ReusePolicy { get; set; }
// workflow tags
// etc.
}
public class ReusePolicy
{
public WorkflowState[] PermittedStates { get; set; }
public OnPolicyViolation PolicyViolationAction { get; set; }
}
public enum WorkflowState
{
Pending,
Running,
Completed,
Failed,
Paused,
Terminated
}
public enum PolicyViolationAction
{
Silent | Skip | Supress,
Throw
}
The default behaviour when ScheduleNewWorkflowOptions
has not been specified would be the equivalent of :
new ScheduleNewWorkflowOptions
{
ReusePolicy = new ReusePolicy
{
// No reuse, therefor maximum safety against accidentally rerunning a
// workflow and performing potentially duplicate / undesirable side-effects
PermittedStates = {},
// Maximum awareness to devs/operators that re-use is not occurring by
// throwing, they can choose to suppress if this is noisy
OnPolicyViolation = PolicyViolationAction.Throw
}
}
The Exception thrown by PolicyViolationAction.Throw
can be polite and hint to the dev/operator that supression options are available and the allowed Reuse states can be specified.
InvalidOperationException : Reuse of this workflow instance ID is not permitted. Specify a custom 'ReusePolicy' to alter the conditions that reuse is permitted, and how policy violations are handled.
For @Clintsigner
use-case, the option would be :
daprWorkflowClient.ScheduleNewWorkflowAsync(
name : ...,
instanceId : ...,
options : new ScheduleNewWorkflowOptions {
reusePolicy : new ReusePolicy {
PermittedStates = { WorkflowState.Running, ... }
}});
This would permit an existing running/etc workflow to be replaced by another
I like the idea of moving the options to an actual options class. Definitely allows for future growth without breaking changes. @olitomlinson proposal of a re-use policy also looks good. The only thing I'd change at this time is change the PermittedStates
to be an enum flags rather than an array and possibly rename it to Conditions
.
[Flags]
public enum WorkflowState
{
None = 0
Pending = 1,
Running = 2,
Completed = 4,
Failed = 8,
Paused = 16,
Terminated = 32,
All = Pending | Running | Completed | Failed | Paused | Terminated
}
By using enum flags the conditions can be merged and defaults can be included, such as an All
option.
daprWorkflowClient.ScheduleNewWorkflowAsync(
name : ...,
instanceId : ...,
reusePolicy : new ReusePolicy {
Conditions = WorkflowState.All
});
The user can also use it in a fine grain manner by cherry picking specific conditions .
daprWorkflowClient.ScheduleNewWorkflowAsync(
name : ...,
instanceId : ...,
reusePolicy : new ReusePolicy {
Conditions = WorkflowState.Completed | WorkflowState.Failed
});
This one wouldn't allow re-use and would be the default condition.
daprWorkflowClient.ScheduleNewWorkflowAsync(
name : ...,
instanceId : ...,
reusePolicy : new ReusePolicy {
Conditions = WorkflowState.None
});
As to the DelayStart
option, I think it would be great to still include it as part of the scheduling rather than relying on workflows always needing the CreateTimer
internally.
Adding it would close https://github.com/dapr/dapr/issues/6450
@clintsinger
The only thing I'd change at this time is change the PermittedStates to be an enum flags rather than an array and possibly rename it to Conditions.
Good suggestions, I'm onboard.
As to the DelayStart option, I think it would be great to still include it as part of the scheduling rather than relying on workflows always needing the CreateTimer internally.
I agree. Tucking the StartTimeUtc
property into the options would make sense, if thats possible.
daprWorkflowClient.ScheduleNewWorkflowAsync(
name : ...,
instanceId : ...,
options : new ScheduleNewWorkflowOptions {
StartTimeUtc : DateTime.UtcNow().AddMinutes(5),
ReusePolicy : new ReusePolicy {
...
}
});
My only question here is at the point of performing the above operation, the runtime would have to 'claim' that particular Workflow ID (or not, and throw an exception etc) -- So what would the runtime status of the workflow be during the 5 minute window before the Workflow actually starts? scheduled
?
Perhaps instead of DelayStart
being a DateTime
it could be a TimeSpan
to match what is in CreateTimer
?
As for the state I think the Pending
state would be sufficient. Pending
could also be renamed to Scheduled
.
I'm not sure where I got the DelayStart
name as the startTime
parameter, but either way I still think we should keep it but have it act as if there was a CreateTimer
with a TimeSpan
.
Probably also need a similar option for child workflows, or continue as new workflows.
Describe the feature
This proposal applies to the
DaprWorkflowClient.ScheduleNewWorkflowAsync
method, which is specific to the Dapr Workflow engine. It doesn't apply toDaprClient.StartWorkflowAsync
, which is the "building block" API that is engine agnostic.The current signature looks like this:
However, it should ideally be changed to better align with Microsoft's Azure SDK guidelines for clients since those guidelines represent general best practices and many .NET developers will find them familiar.
Since this method could be considered complex, or could turn into a complex method, it should be changed to look something like this:
This should be done for the beta release since it's a breaking change and likely represents the path forward.
Release Note
RELEASE NOTE: UPDATE New (breaking) method signature for starting workflows