Skip to content

Remove skip_generate_workflow_task from signal APIs #476

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

Merged
alexshtin merged 2 commits into from
Nov 18, 2024
Merged

Remove skip_generate_workflow_task from signal APIs #476

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
cad0299
Remove SkipGenerateWorkflowTask from signal APIs
alexshtin Nov 14, 2024
d6d0a99
Deprecate history field
alexshtin Nov 15, 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
3 changes: 2 additions & 1 deletion buf.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ build:
- google
breaking:
use:
- WIRE_JSON
# TODO (alex): revert it back to WIRE_JSON in follow up PR.
- WIRE
ignore:
- google
lint:
Expand Down
14 changes: 1 addition & 13 deletions openapi/openapiv2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5750,11 +5750,7 @@
},
"workflowStartDelay": {
"type": "string",
"description": "Time to wait before dispatching the first workflow task. Cannot be used with `cron_schedule`.\nNote that the signal will be delivered with the first workflow task. If the workflow gets\nanother SignalWithStartWorkflow before the delay and `skip_generate_workflow_task` is false\nor not set, a workflow task will be dispatched immediately and the rest of the delay period\nwill be ignored, even if that request also had a delay. Signal via SignalWorkflowExecution\nwill not unblock the workflow."
},
"skipGenerateWorkflowTask": {
"type": "boolean",
"description": "Indicates that a new workflow task should not be generated when this signal is received."
"description": "Time to wait before dispatching the first workflow task. Cannot be used with `cron_schedule`.\nNote that the signal will be delivered with the first workflow task. If the workflow gets\nanother SignalWithStartWorkflow before the delay a workflow task will be dispatched immediately\nand the rest of the delay period will be ignored, even if that request also had a delay.\nSignal via SignalWorkflowExecution will not unblock the workflow."
},
"userMetadata": {
"$ref": "#/definitions/v1UserMetadata",
Expand Down Expand Up @@ -5802,10 +5798,6 @@
"$ref": "#/definitions/v1Header",
"description": "Headers that are passed with the signal to the processing workflow.\nThese can include things like auth or tracing tokens."
},
"skipGenerateWorkflowTask": {
"type": "boolean",
"description": "Indicates that a new workflow task should not be generated when this signal is received."
},
"links": {
"type": "array",
"items": {
Expand Down Expand Up @@ -12166,10 +12158,6 @@
"$ref": "#/definitions/v1Header",
"description": "Headers that were passed by the sender of the signal and copied by temporal \nserver into the workflow task."
},
"skipGenerateWorkflowTask": {
"type": "boolean",
"description": "Indicates the signal did not generate a new workflow task when received."
},
"externalWorkflowExecution": {
"$ref": "#/definitions/v1WorkflowExecution",
"description": "When signal origin is a workflow execution, this field is set."
Expand Down
16 changes: 3 additions & 13 deletions openapi/openapiv3.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8272,13 +8272,9 @@ components:
description: |-
Time to wait before dispatching the first workflow task. Cannot be used with `cron_schedule`.
Note that the signal will be delivered with the first workflow task. If the workflow gets
another SignalWithStartWorkflow before the delay and `skip_generate_workflow_task` is false
or not set, a workflow task will be dispatched immediately and the rest of the delay period
will be ignored, even if that request also had a delay. Signal via SignalWorkflowExecution
will not unblock the workflow.
skipGenerateWorkflowTask:
type: boolean
description: Indicates that a new workflow task should not be generated when this signal is received.
another SignalWithStartWorkflow before the delay a workflow task will be dispatched immediately
and the rest of the delay period will be ignored, even if that request also had a delay.
Signal via SignalWorkflowExecution will not unblock the workflow.
userMetadata:
allOf:
- $ref: '#/components/schemas/UserMetadata'
Expand Down Expand Up @@ -8329,9 +8325,6 @@ components:
description: |-
Headers that are passed with the signal to the processing workflow.
These can include things like auth or tracing tokens.
skipGenerateWorkflowTask:
type: boolean
description: Indicates that a new workflow task should not be generated when this signal is received.
links:
type: array
items:
Expand Down Expand Up @@ -9689,9 +9682,6 @@ components:
allOf:
- $ref: '#/components/schemas/Header'
description: "Headers that were passed by the sender of the signal and copied by temporal \n server into the workflow task."
skipGenerateWorkflowTask:
type: boolean
description: Indicates the signal did not generate a new workflow task when received.
externalWorkflowExecution:
allOf:
- $ref: '#/components/schemas/WorkflowExecution'
Expand Down
3 changes: 1 addition & 2 deletions temporal/api/history/v1/message.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,7 @@ message WorkflowExecutionSignaledEventAttributes {
// Headers that were passed by the sender of the signal and copied by temporal
// server into the workflow task.
temporal.api.common.v1.Header header = 4;
// Indicates the signal did not generate a new workflow task when received.
bool skip_generate_workflow_task = 5;
reserved 5;
Copy link
Member

@cretz cretz Nov 14, 2024

Choose a reason for hiding this comment

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

I do not believe this is a safe/compatible change. We have an obligation to have JSON/field-name compatibility, not just proto binary compatibility, especially for history but also for RPC requests since we have an HTTP API. Can we just document that it's deprecated/ignored?

// When signal origin is a workflow execution, this field is set.
temporal.api.common.v1.WorkflowExecution external_workflow_execution = 6;
}
Expand Down
13 changes: 5 additions & 8 deletions temporal/api/workflowservice/v1/request_response.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,8 +654,7 @@ message SignalWorkflowExecutionRequest {
// Headers that are passed with the signal to the processing workflow.
// These can include things like auth or tracing tokens.
temporal.api.common.v1.Header header = 8;
// Indicates that a new workflow task should not be generated when this signal is received.
bool skip_generate_workflow_task = 9;
reserved 9;
Copy link
Member

@cretz cretz Nov 15, 2024

Choose a reason for hiding this comment

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

I think we need to do the same thing here as we did with the history, we need to deprecate/document but not remove the field. These things are part of the HTTP API and need to be JSON compatible.


// Links to be associated with the WorkflowExecutionSignaled event.
repeated temporal.api.common.v1.Link links = 10;
Expand Down Expand Up @@ -708,13 +707,11 @@ message SignalWithStartWorkflowExecutionRequest {
temporal.api.common.v1.Header header = 19;
// Time to wait before dispatching the first workflow task. Cannot be used with `cron_schedule`.
// Note that the signal will be delivered with the first workflow task. If the workflow gets
// another SignalWithStartWorkflow before the delay and `skip_generate_workflow_task` is false
// or not set, a workflow task will be dispatched immediately and the rest of the delay period
// will be ignored, even if that request also had a delay. Signal via SignalWorkflowExecution
// will not unblock the workflow.
// another SignalWithStartWorkflow before the delay a workflow task will be dispatched immediately
// and the rest of the delay period will be ignored, even if that request also had a delay.
// Signal via SignalWorkflowExecution will not unblock the workflow.
google.protobuf.Duration workflow_start_delay = 20;
// Indicates that a new workflow task should not be generated when this signal is received.
bool skip_generate_workflow_task = 21;
reserved 21;
// Metadata on the workflow if it is started. This is carried over to the WorkflowExecutionInfo
// for use by user interfaces to display the fixed as-of-start summary and details of the
// workflow.
Expand Down