Add SignalWithStart #621
Add SignalWithStart #621
Conversation
| @@ -962,6 +962,22 @@ struct SignalWorkflowExecutionRequest { | |||
| 70: optional binary control | |||
| } | |||
|
|
|||
| struct SignalWithStartWorkflowExecutionRequest { | |||
There was a problem hiding this comment.
out of curiosity, why not just add the signal part to existing start workflow API?
There was a problem hiding this comment.
That's a decision after design meeting. We prefer creating new API (and related requests) for such new cases. e.g. completeActivityByID is new instead of reuse.
But we still try best to reuse as much as possible.
| @@ -470,8 +470,9 @@ func (h *Handler) StartWorkflowExecution(ctx context.Context, | |||
|
|
|||
| response, err2 := engine.StartWorkflowExecution(wrappedRequest) | |||
| if err2 != nil { | |||
| h.updateErrorMetric(metrics.HistoryStartWorkflowExecutionScope, h.convertError(err2)) | |||
| return nil, h.convertError(err2) | |||
| tmpErr := h.convertError(err2) | |||
service/history/historyEngine.go
Outdated
| if err0 != nil { | ||
| if _, ok := err0.(*workflow.EntityNotExistsError); ok { | ||
| isBrandNew = true | ||
| goto Start_And_Signal |
There was a problem hiding this comment.
agree it might feel uncomfortable, removed.
service/history/historyEngine.go
Outdated
| } | ||
|
|
||
| // Generate first decision task event. | ||
| taskList := *request.TaskList.Name |
| } | ||
| msBuilder.executionInfo.LastFirstEventID = startedEvent.GetEventId() | ||
|
|
||
| createWorkflow := func(isBrandNew bool, prevRunID string) (string, error) { |
There was a problem hiding this comment.
this func does not take the workflow ID reuse policy in consideration.
moreover, it is hard to actually maintain a second set of start workflow logic.
so could we actually merge the signal & start to start workflow API?
There was a problem hiding this comment.
I think merging the implementation does make sense. But merging the APIs is not.
There was a problem hiding this comment.
i believe, history side start && start & signal APIs can be merged since it is not public.
There was a problem hiding this comment.
I thought about merge/reuse "start" but there is no good way to achieve it. "start" includes all complicate but not needed logic (like child, restart etc) for sigWithStart; and sigWithStart need start (just start) and signal in transaction.
I suggest keep sigWithStart separate from "start"
service/frontend/handler.go
Outdated
| return nil, wh.error(createServiceBusyError(), scope) | ||
| } | ||
|
|
||
| if signalWithStartRequest.Domain == nil { |
There was a problem hiding this comment.
do you need to check if Domain == ""?
There was a problem hiding this comment.
Added. Will check and change all APIs in another request. #581
service/frontend/handler.go
Outdated
| return nil, wh.error(&gen.BadRequestError{Message: "WorkflowId is not set on request."}, scope) | ||
| } | ||
|
|
||
| if signalWithStartRequest.SignalName == nil { |
service/history/historyEngine.go
Outdated
|
|
||
| // Start workflow and signal | ||
| request := getStartRequest(sRequest) | ||
| if request.ExecutionStartToCloseTimeoutSeconds == nil || *request.ExecutionStartToCloseTimeoutSeconds <= 0 { |
There was a problem hiding this comment.
maybe worth to reuse those parameter check with StartWorkflow()
There was a problem hiding this comment.
good suggestion, changed.
* Add SignalWithStart * rebase
Currently, Cadence support sending signals to running workflows using Client SingalWorkflow APIs. However, if workflow is not running, either not started or already closed, SignalWorkflow will return error. So if user wants to make sure a signal is delivered, user needs to catch the error, start the workflow and then send signal again. It’s not convenient for user, also it requires additional calls to Cadence.
This CR create new API SignalWithStart to ensure signal delivery.
In this way, user can perform all these signal-start-signal in one call.