Skip to content
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.

Sign up for GitHub

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

Jump to bottom

allow client to request keep scheduled workflow id as is. #1393

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

allow client to request keep scheduled workflow id as is. #1393

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
acca81d
allow client to request keep scheduled workflow id as is.
ast2023 Feb 22, 2024
d9fcae1
adding test
ast2023 Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions internal/internal_schedule_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
taskqueuepb "go.temporal.io/api/taskqueue/v1"
workflowpb "go.temporal.io/api/workflow/v1"
"go.temporal.io/api/workflowservice/v1"

"go.temporal.io/sdk/converter"
"go.temporal.io/sdk/log"
)
Expand Down Expand Up @@ -136,9 +137,10 @@ func (w *workflowClientInterceptor) CreateSchedule(ctx context.Context, in *Sche
Spec: convertToPBScheduleSpec(&in.Options.Spec),
Action: action,
Policies: &schedulepb.SchedulePolicies{
OverlapPolicy: in.Options.Overlap,
CatchupWindow: catchupWindow,
PauseOnFailure: in.Options.PauseOnFailure,
OverlapPolicy: in.Options.Overlap,
CatchupWindow: catchupWindow,
PauseOnFailure: in.Options.PauseOnFailure,
KeepOriginalWorkflowId: in.Options.KeepOriginalWorkflowID,
},
State: &schedulepb.ScheduleState{
Notes: in.Options.Note,
Expand Down
4 changes: 4 additions & 0 deletions internal/schedule_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this only applies if the action is ScheduleWorkflowAction is there a reason you didn't put the field on the action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// appended to the scheduled workflow id.
KeepOriginalWorkflowID bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right. Protobuf allows us to ignore fields which we do not recognize. So server will just ignore the new field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably document this option may not apply depending on the server version, ideally say what server version this option will be respected

}

// ScheduleWorkflowExecution contains details on a workflows execution stared by a schedule.
Expand Down
Loading