buffer events when decision task is inflight #386

yiminc-zz · 2017-10-21T04:13:51Z

No description provided.

coveralls · 2017-10-21T08:07:25Z

Coverage increased (+0.08%) to 66.48% when pulling a44aeeb on yiminc:buffer_events into cbc769d on uber:master.

coveralls · 2017-10-23T04:35:04Z

Coverage increased (+0.09%) to 66.492% when pulling d1256b3 on yiminc:buffer_events into cbc769d on uber:master.

vancexu · 2017-10-23T16:59:12Z

service/history/historyBuilder.go

-	emptyEventID int64 = -23
+	firstEventID    int64 = 1
+	emptyEventID    int64 = -23
+	bufferedEventID int64 = -123


Just curious why choose number -123? (also why emptyEventID -23)

I just pick a random number that is unique and is less than 0.

coveralls · 2017-10-23T19:58:17Z

Coverage increased (+0.08%) to 66.48% when pulling abfe317 on yiminc:buffer_events into cbc769d on uber:master.

samarabbas · 2017-10-24T23:26:52Z

common/persistence/cassandraPersistence.go

+func (d *cassandraPersistence) updateBufferedEvents(batch *gocql.Batch, newBufferedEvents []*SerializedHistoryEventBatch,
+	clearBufferedEvents bool, domainID, workflowID, runID string, condition int64, rangeID int64) {
+
+	if clearBufferedEvents {


newBufferedEvents and clearBufferedEvents should be mutually exclusive. Let's have some validation here to make sure only one is set and convert this piece of code into if-else block.

samarabbas · 2017-10-24T23:30:32Z

common/persistence/cassandraPersistence.go

@@ -942,6 +964,14 @@ func (d *cassandraPersistence) GetWorkflowExecution(request *GetWorkflowExecutio
 	}
 	state.RequestCancelInfos = requestCancelInfos

+	eList := result["buffered_events_list"].([]map[string]interface{})


Can you refactor this logic in a helper method. For example look at 'createTimerTaskInfo'. We create a helper method to deserialize each of these custom types.

the logic is already put in a help function createSerializedHistoryEventBatch().

samarabbas · 2017-10-24T23:32:53Z

common/persistence/cassandraPersistence.go

+	eventBatch := &SerializedHistoryEventBatch{EncodingType: common.EncodingTypeJSON}
+	for k, v := range result {
+		switch k {
+		case "version":


I'm fine with the default but why are you not reading the encoding_type?

since the value is already set to JSON when create the struct in this method, so there is no need to read the encoding_type, unless we support other encoding type.

samarabbas · 2017-10-25T00:46:36Z

service/history/historyEngine.go

@@ -573,7 +573,7 @@ Update_History_Loop:
 		var failCause workflow.DecisionTaskFailedCause
 		var err error
 		completedID := *completedEvent.EventId
-		hasUnhandledEvents := ((completedID - startedID) > 1)
+		hasUnhandledEvents := (msBuilder.GetNextEventID() - completedID) > 1


Two things:

We should first append all the decisions as part of decision task completed before flushing buffer to history events.

Then this check could just be msBuilder.HasBufferedEvents.

will update

samarabbas · 2017-10-25T00:48:32Z

service/history/mutableStateBuilder.go

@@ -58,6 +58,10 @@ type (
 		updateRequestCancelInfos    []*persistence.RequestCancelInfo         // Modified RequestCancel Infos since last update
 		deleteRequestCancelInfo     *int64                                   // Deleted RequestCancel Info since last update

+		persistedBufferedEvents []*persistence.SerializedHistoryEventBatch // buffered history events that are already persisted


Can we use the same naming convention like for other mutable state items.
bufferedEvents and updateBufferedEvents

will update

samarabbas · 2017-10-25T00:50:14Z

service/history/mutableStateBuilder.go

@@ -75,6 +79,8 @@ type (
 		updateChildExecutionInfos []*persistence.ChildExecutionInfo
 		deleteChildExecutionInfo  *int64
 		continueAsNew             *persistence.CreateWorkflowExecutionRequest
+		newBufferedEvents         []*persistence.SerializedHistoryEventBatch


Does this needs to be an array? Every update should create a single batch of serialized event.

this is to make unit test pass

samarabbas · 2017-10-25T00:57:35Z

service/history/mutableStateBuilder.go

+	// put new events into 2 buckets:
+	//  1) if the event was added while there was in-flight decision, then put it in buffered bucket
+	//  2) otherwise, put it in committed bucket
+	var newBufferedEvents []*workflow.HistoryEvent


I guess this could be simplified as we can never have partial buffered or committed events in the same update.

but you still need to check if this is buffered or committed batch.

I tried to put the events into buffered or committed based on first event's event id, but it would break lots of unit tests, mostly because we have the tests that addDecisionTaskStarted, then add some other events, then add decisionTaskCompleted. I would spend more time to update those tests, but i feel it is not worth it.
All other comments have been addressed.

coveralls · 2017-10-25T04:32:55Z

Coverage increased (+0.07%) to 66.476% when pulling f1f67f0 on yiminc:buffer_events into cbc769d on uber:master.

coveralls · 2017-10-25T05:07:03Z

Coverage increased (+0.06%) to 66.46% when pulling 1b66aa0 on yiminc:buffer_events into cbc769d on uber:master.

coveralls · 2017-10-26T05:27:43Z

Coverage decreased (-0.03%) to 66.466% when pulling d8fc3c9 on yiminc:buffer_events into 1fb0a3d on uber:master.

yiminc-zz requested review from venkat1109, mfateev and samarabbas October 21, 2017 04:13

yiminc-zz force-pushed the buffer_events branch from c16de57 to a44aeeb Compare October 21, 2017 07:54

yiminc-zz force-pushed the buffer_events branch from a44aeeb to d1256b3 Compare October 23, 2017 01:15

vancexu reviewed Oct 23, 2017

View reviewed changes

buffer events when decision task is inflight

abfe317

yiminc-zz force-pushed the buffer_events branch from d1256b3 to abfe317 Compare October 23, 2017 19:44

samarabbas reviewed Oct 25, 2017

View reviewed changes

yiminc-zz force-pushed the buffer_events branch from c7f2ad7 to f1f67f0 Compare October 25, 2017 04:19

update based on review comments

1b66aa0

yiminc-zz force-pushed the buffer_events branch from f1f67f0 to 1b66aa0 Compare October 25, 2017 04:54

samarabbas approved these changes Oct 25, 2017

View reviewed changes

Merge branch 'master' into buffer_events

d8fc3c9

yiminc-zz merged commit d79841b into cadence-workflow:master Oct 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer events when decision task is inflight #386

buffer events when decision task is inflight #386

yiminc-zz commented Oct 21, 2017

coveralls commented Oct 21, 2017

coveralls commented Oct 23, 2017

vancexu Oct 23, 2017

yiminc-zz Oct 23, 2017

coveralls commented Oct 23, 2017

samarabbas Oct 24, 2017

samarabbas Oct 24, 2017

yiminc-zz Oct 25, 2017

samarabbas Oct 24, 2017

yiminc-zz Oct 25, 2017

samarabbas Oct 25, 2017

yiminc-zz Oct 25, 2017

samarabbas Oct 25, 2017

yiminc-zz Oct 25, 2017

samarabbas Oct 25, 2017

yiminc-zz Oct 25, 2017

samarabbas Oct 25, 2017

yiminc-zz Oct 25, 2017

yiminc-zz Oct 25, 2017

coveralls commented Oct 25, 2017

coveralls commented Oct 25, 2017

coveralls commented Oct 26, 2017

buffer events when decision task is inflight #386

buffer events when decision task is inflight #386

Conversation

yiminc-zz commented Oct 21, 2017

coveralls commented Oct 21, 2017

coveralls commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 23, 2017

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

coveralls commented Oct 25, 2017

coveralls commented Oct 25, 2017

coveralls commented Oct 26, 2017