allow client to request keep scheduled workflow id as is. #1393
Conversation
|
|
||
| // KeepOriginalWorkflowID - If true, and the action would start a workflow, a timestamp will not be | ||
| // appended to the scheduled workflow id. | ||
| KeepOriginalWorkflowID bool |
There was a problem hiding this comment.
What would happen if users try to set this to true and call CreateSchedule on an older server version that doesn't support this field? I suspect it will succeed, but ignore the call fields correct?
There was a problem hiding this comment.
I think you are right. Protobuf allows us to ignore fields which we do not recognize. So server will just ignore the new field.
There was a problem hiding this comment.
We should probably document this option may not apply depending on the server version, ideally say what server version this option will be respected
| @@ -363,6 +363,10 @@ type ( | |||
| // | |||
| // [Visibility]: https://docs.temporal.io/visibility | |||
| TypedSearchAttributes SearchAttributes | |||
|
|
|||
| // KeepOriginalWorkflowID - If true, and the action would start a workflow, a timestamp will not be | |||
There was a problem hiding this comment.
Since this only applies if the action is ScheduleWorkflowAction is there a reason you didn't put the field on the action?
There was a problem hiding this comment.
Initially it was in ScheduleAction. Then @dnr pointed out that Policy is the more logical place for this field. There is no Policy in the SDK ScheduleOptions. So I left the field at the options top level.
How can users set Policy from the workflow code?
There was a problem hiding this comment.
I am not sure why policy is more logical if this only applies to one type of action. @dnr do you expect this option to apply to other type of actions?
There was a problem hiding this comment.
We don't really have a distinct type for the start workflow action. Maybe we should have. I think "policies" make sense as general configuration that can affect schedule execution.
For other actions: suppose we had an action to start a top-level activity. I think top-level activities would still have names, so it could apply there too. Same for starting a nexus operation, though I'm not sure about naming there.
There was a problem hiding this comment.
hmm if this option is meant to apply to other future actions like top level activity of nexus operation then I think we should change the name to not include Workflow since that is specific to one type of action.
What was changed
Add option to keep scheduled workflow id as is.
Why?
User request.
Checklist
Not yet tested.