bugfix: request cancel info should be pass down to persistence layer, add functionality to allow workflow signal and cancellation to specify target child workflow only #544
Conversation
wxing1292
force-pushed
the
cancellation
branch
3 times, most recently
from
654bf89 to
48b7d50
Compare
wxing1292
changed the title
wxing1292
force-pushed
the
cancellation
branch
4 times, most recently
from
5cc6d26 to
8b4ff93
Compare
bugfix: cancel workflow control is not used add functionality to allow workflow signal and cancellation to specify target child workflow only
wxing1292
force-pushed
the
cancellation
branch
from
8b4ff93 to
6c41f31
Compare
| @@ -550,6 +553,7 @@ struct ExternalWorkflowExecutionCancelRequestedEventAttributes { | |||
| 10: optional i64 (js.type = "Long") initiatedEventId | |||
| 20: optional string domain | |||
| 30: optional WorkflowExecution workflowExecution | |||
| 40: optional binary control | |||
There was a problem hiding this comment.
why do we need control here?
There was a problem hiding this comment.
because we cannot use the workflow ID as unique identifier for workflow cancellation.
so, we must come up with a "ID", and control is the ID.
the reason i use "binary control" instead of "int64 id" is to follow the existing pattern
| @@ -919,6 +924,7 @@ struct RequestCancelWorkflowExecutionRequest { | |||
| 20: optional WorkflowExecution workflowExecution | |||
| 30: optional string identity | |||
| 40: optional string requestId | |||
| 50: optional binary control | |||
| @@ -0,0 +1,9 @@ | |||
| { | |||
| "CurrVersion": "0.6", | |||
There was a problem hiding this comment.
version 0.5 is not released yet. Can you have this schema change as part of 0.5?
There was a problem hiding this comment.
version 0.5 is used specifically for cross DC testing. and 0.5 binary is deployed to 1 host.
service/history/historyBuilder.go
Outdated
| @@ -659,17 +659,18 @@ func (b *historyBuilder) newRequestCancelExternalWorkflowExecutionInitiatedEvent | |||
| attributes.DecisionTaskCompletedEventId = common.Int64Ptr(decisionTaskCompletedEventID) | |||
| attributes.Domain = common.StringPtr(*request.Domain) | |||
| attributes.WorkflowExecution = &workflow.WorkflowExecution{ | |||
| WorkflowId: common.StringPtr(*request.WorkflowId), | |||
| RunId: common.StringPtr(*request.RunId), | |||
| WorkflowId: common.StringPtr(request.GetWorkflowId()), | |||
There was a problem hiding this comment.
Just assign the pointer directly. No need to dereference.
WorkflowId: request.WorkflowId
service/history/historyEngine.go
Outdated
| @@ -978,11 +981,10 @@ Update_History_Loop: | |||
| break Process_Decision_Loop | |||
| } | |||
|
|
|||
| foreignDomainEntry, err := e.domainCache.GetDomain(*attributes.Domain) | |||
| foreignDomainEntry, err := e.domainCache.GetDomain(attributes.GetDomain()) | |||
There was a problem hiding this comment.
I think we should treat domain on attributes as optional and use the current domain if one is not provided.
| @@ -229,6 +229,14 @@ func (t *transferQueueProcessorImpl) processTransferTasks(tasksCh chan<- *persis | |||
| return | |||
| } | |||
|
|
|||
| // here we may have a serious bug | |||
There was a problem hiding this comment.
This comment seems little misleading. Let's talk about this tomorrow.
bugfix: request cancel info should be pass down to persistence layer
make transferTaskTypeTransferTargetRunID used in persistence layer only
enable cancel current workflow decision
client side change: cadence-workflow/cadence-go-client#401
solve #512