Add signal external workflow decision #485

vancexu · 2017-12-29T21:04:46Z

related #429
client side change: cadence-workflow/cadence-go-client#329

samarabbas

Reviewed IDL and schema changes. Will do the engine changes tomorrow.

samarabbas · 2018-01-05T00:26:56Z

idl/github.com/uber/cadence/shared.thrift

@@ -109,6 +110,9 @@ enum EventType {
  RequestCancelExternalWorkflowExecutionFailed,
  ExternalWorkflowExecutionCancelRequested,
  MarkerRecorded,
+  SignalExternalWorkflowExecutionInitiated,


This will break existing clients. Please add new values at the end.

Good to know. Thanks.

samarabbas · 2018-01-05T00:29:21Z

idl/github.com/uber/cadence/shared.thrift

@@ -132,6 +136,7 @@ enum DecisionTaskFailedCause {
  BAD_COMPLETE_WORKFLOW_EXECUTION_ATTRIBUTES,
  BAD_FAIL_WORKFLOW_EXECUTION_ATTRIBUTES,
  BAD_CANCEL_WORKFLOW_EXECUTION_ATTRIBUTES,
+  BAD_SIGNAL_WORKFLOW_EXECUTION_ATTRIBUTES,


Add new enums towards the very end.

samarabbas · 2018-01-05T00:55:37Z

idl/github.com/uber/cadence/shared.thrift

+  60: optional binary control
+}
+
+struct ExternalWorkflowExecutionSignalRequestedEventAttributes {


Probably rename it to ExternalWorkflowExecutionSignaledEventAttributes

I will probably not change this, because the event type is EventTypeExternalWorkflowExecutionSignalRequested. If I rename this to ExternalWorkflowExecutionSignaledEventAttributes, then I probably need to change the event name to EventTypeExternalWorkflowExecutionSignaled, but we try to be consistent with SWF so wouldn't change event name.

I'm still open minded, let me know if you still prefer new name

SWF is using ExternalWorkflowExecutionSignaled.

samarabbas · 2018-01-05T00:59:10Z

idl/github.com/uber/cadence/shared.thrift

+struct DeleteWorkflowExecutionSignalRequest {
+  10: optional string domain
+  20: optional WorkflowExecution workflowExecution
+  30: optional string identity


Probably identity does not make sense here.

True, removed.

samarabbas · 2018-01-05T01:02:33Z

idl/github.com/uber/cadence/history.thrift

+  * DeleteWorkflowExecutionSignal is used to delete a signal request ID that was previously recorded.  This is currently
+  * used to clean execution info when signal decision finished.
+  **/
+  void DeleteWorkflowExecutionSignal(1: DeleteWorkflowExecutionSignalRequest deleteRequest)


Let's rename this to RemoveSignalMutableState

samarabbas · 2018-01-05T01:21:00Z

schema/cadence/versioned/v0.2/add_signal_decision.cql

@@ -0,0 +1,9 @@
+CREATE TYPE signal_info (


This should go into v0.3. We have already released v0.2.

Will move when rebase

samarabbas · 2018-01-05T01:33:22Z

common/persistence/dataInterfaces.go

+		TargetDomainID   string
+		TargetWorkflowID string
+		TargetRunID      string
+		ScheduleID       int64


Maybe rename to InitiatedID

samarabbas · 2018-01-05T01:35:10Z

common/persistence/dataInterfaces.go

+		InitiatedID     int64
+		SignalRequestID string
+		SignalName      string
+		Input           []byte


You probably need to store the entire SignalInitiatedEvent here.

samarabbas · 2018-01-05T01:38:51Z

common/persistence/dataInterfaces.go

@@ -655,6 +686,7 @@ type (
 		GetWorkflowExecution(request *GetWorkflowExecutionRequest) (*GetWorkflowExecutionResponse, error)
 		UpdateWorkflowExecution(request *UpdateWorkflowExecutionRequest) error
 		DeleteWorkflowExecution(request *DeleteWorkflowExecutionRequest) error
+		DeleteSignalRequestedID(request *DeleteWorkflowExecutionSignalRequestedRequest) error


Why this is a new API but DeleteSignalInfo is part of update?

samarabbas · 2018-01-05T01:44:28Z

common/persistence/cassandraPersistence.go

+		`and workflow_id = ? ` +
+		`and run_id = ? ` +
+		`and visibility_ts = ? ` +
+		`and task_id = ? `


This is also updating the mutable state. Make sure it is protected by conditional check on range_id and next_event_id.

changed, protected by using updateWorkflowExecution.

samarabbas · 2018-01-05T18:01:37Z

service/history/historyBuilder.go

+	return event
+}
+
+func (b *historyBuilder) newSignalExternalWorkflowExecutionFailedEvent(decisionTaskCompletedEventID, initiatedEventID int64,


You never set control on this event.

samarabbas · 2018-01-05T18:01:49Z

service/history/historyBuilder.go

+	return event
+}
+
+func (b *historyBuilder) newExternalWorkflowExecutionSignalRequestedEvent(initiatedEventID int64,


You never set control on this event.

samarabbas · 2018-01-05T18:11:03Z

service/history/historyEngine.go

+				transferTasks = append(transferTasks, &persistence.SignalExecutionTask{
+					TargetDomainID:   foreignInfo.ID,
+					TargetWorkflowID: attributes.GetWorkflowId(),
+					TargetRunID:      common.StringDefault(attributes.RunId),


Use the generated getter: attributes.GetRunId()

samarabbas · 2018-01-05T18:14:17Z

service/history/historyEngine.go

@@ -1528,6 +1560,17 @@ func (e *historyEngineImpl) SignalWorkflowExecution(signalRequest *h.SignalWorkf
 				return &workflow.EntityNotExistsError{Message: "Workflow execution already completed."}
 			}

+			// deduplicate by request id for signal decision
+			if request.RequestId != nil {


This can be simplified if you use the generated getter.

Changed, good to know

samarabbas · 2018-01-05T18:16:17Z

service/history/historyEngine.go

@@ -1536,6 +1579,42 @@ func (e *historyEngineImpl) SignalWorkflowExecution(signalRequest *h.SignalWorkf
 		})
 }

+func (e *historyEngineImpl) DeleteWorkflowExecutionSignal(deleteRequest *h.DeleteWorkflowExecutionSignalRequest) error {
+	domainID, err := getDomainUUID(deleteRequest.DomainUUID)


Lets use the updateWorkflowExecution helper defined in historyEngineImpl for making this update.

samarabbas · 2018-01-05T18:38:51Z

service/history/mutableStateBuilder.go

+		return nil
+	}
+
+	if e.DeletePendingSignal(initiatedID) == nil {


What if this is not nil?

Good question. We have some options:

add new PendingSignalNotExistEvent event when err happens

add success (or failed) event no matter err happens or not

add nothing when err happens (current implementation) .

Which one do you like best?

I added log if err happens.

samarabbas · 2018-01-05T18:39:16Z

service/history/mutableStateBuilder.go

+		return nil
+	}
+
+	if e.DeletePendingSignal(initiatedID) == nil {


What is this is not nil?

samarabbas · 2018-01-05T18:44:18Z

service/history/transferQueueProcessor.go

+		},
+	}
+
+	op := func() error {


Can you just create a helper for SignalExecutionWithRetry.

samarabbas · 2018-01-05T18:56:48Z

service/history/transferQueueProcessor.go

+	initiatedEventID := task.ScheduleID
+	ri, isRunning := msBuilder.GetSignalInfo(initiatedEventID)
+	if !isRunning {
+		return nil


You still want to call delete on the target to make sure we don't leak requestIDs in mutable state.

In which case leak can happens?
I found it hard to delete target here because no ri.SignalRequestId can be obtained here.

samarabbas · 2018-01-05T19:02:13Z

service/history/transferQueueProcessor.go

+
+		// Check to see if the error is non-transient, in which case add SignalFailed
+		// event and complete transfer task by setting the err = nil
+		if common.IsServiceNonRetryableError(err) {


Make sure to add a test for expected error types here. Otherwise this transfer task can go into infinite loop.

Added tests for transferQueueProcessor

coveralls · 2018-01-10T00:14:38Z

Coverage increased (+0.7%) to 67.261% when pulling 09c0a09 on signaldi into 7a51af2 on master.

coveralls · 2018-01-10T02:57:15Z

Coverage increased (+0.8%) to 67.381% when pulling 21e5cac on signaldi into 7a51af2 on master.

…ributes

…cessor

coveralls · 2018-01-20T01:33:38Z

Coverage increased (+0.6%) to 67.283% when pulling 3a996e2 on signaldi into 7c79aaf on master.

vancexu requested review from mfateev, samarabbas, wxing1292, yiminc-zz and madhuravi December 29, 2017 21:04

samarabbas suggested changes Jan 5, 2018

View reviewed changes

samarabbas reviewed Jan 5, 2018

View reviewed changes

vancexu force-pushed the signaldi branch from 8b07fd1 to 09c0a09 Compare January 9, 2018 22:21

vancexu added 12 commits January 19, 2018 16:57

Add signal external workflow decision

7d848ae

replace inproper usage of dereference

11b6ff9

Add control field to ExternalWorkflowExecutionSignalRequestedEventAtt…

29da391

…ributes

Address comments

8184bd1

Add tests for transferQueueProcessor

06a809f

rebase

8f90e12

Change event name and attribute type

3002bd3

Process empty runID

3695cf9

increase test timeout to 10min

bdb9d6c

log err in msbuilder, add TODO for removeSignalMS in transferQueuePro…

a9d612a

…cessor

Fix lint warning

2f10087

rebase to master

f29dec1

vancexu force-pushed the signaldi branch from 2ec0aae to f29dec1 Compare January 20, 2018 01:15

protect deleteSingalRequested with condition

3a996e2

samarabbas approved these changes Jan 22, 2018

View reviewed changes

vancexu merged commit bd9eb9f into master Jan 22, 2018

vancexu deleted the signaldi branch January 22, 2018 21:11

Add signal external workflow decision #485

Add signal external workflow decision #485

Conversation

vancexu commented Dec 29, 2017 • edited Loading

samarabbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancexu Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

coveralls commented Jan 20, 2018 • edited Loading

vancexu commented Dec 29, 2017 •

edited

Loading

vancexu Jan 9, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading