Skip to content
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.

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

history service should do event reordering making sure corresponding … #601

Merged
merged 5 commits into from
Mar 13, 2018

Conversation

wxing1292
Copy link
Contributor

@wxing1292 wxing1292 commented Mar 8, 2018

…events for decision will have exactly the same order and no irrelevant event will be inserted in between, so client can predict the event ID of a corresponding decision.

tested using canary

counterpart: #600

@coveralls
Copy link

coveralls commented Mar 8, 2018

Coverage Status

Coverage increased (+0.1%) to 64.161% when pulling a67b4e7 on decision-order-2 into 35bf3de on master.

// So both should not be buffered. Ref: historyEngine, search for "workflow.DecisionTypeCancelTimer"
workflow.EventTypeTimerCanceled,
workflow.EventTypeCancelTimerFailed,
// workflow.EventTypeWorkflowExecutionFailed,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this.

@@ -378,26 +378,48 @@ func (e *mutableStateBuilder) createNewHistoryEvent(eventType workflow.EventType
}

func (e *mutableStateBuilder) shouldBufferEvent(eventType workflow.EventType) bool {
if !e.HasInFlightDecisionTask() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is removing this safe? This changes the logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from what i see in the code logic
prev:
if no decision on the fly, just assign the event ID
if decision on the fly, postpone until decision is done.

now:
if events in the list, assign event ID
if events not in the list, assign event ID when there is no decision on the fly, by FlushBufferedEvents():
https://github.com/uber/cadence/blob/master/service/history/mutableStateBuilder.go#L157

So let us consider several cases:

  1. no decision on the fly
    prev: event ID will be assign immediately
    now: events in the list will be assigned an ID immediately, events not in the list will be assigned an ID when FlushBufferedEvents is called, which is before persistence of mutable state.

  2. decision on the fly
    prev: only several event will assign ID immediately
    now: in addition, other events will be assign ID immediately:
    category a:
    EventTypeDecisionTaskScheduled
    EventTypeDecisionTaskStarted
    EventTypeWorkflowExecutionStarted
    category b:
    EventTypeActivityTaskScheduled
    EventTypeActivityTaskCancelRequested
    EventTypeTimerStarted
    EventTypeTimerCanceled
    EventTypeCancelTimerFailed
    EventTypeRequestCancelExternalWorkflowExecutionInitiated
    EventTypeMarkerRecorded
    EventTypeStartChildWorkflowExecutionInitiated
    EventTypeSignalExternalWorkflowExecutionInitiated

category a is obvious
category b is the events generated by decision, which can only happen when a decision is complete.

so overall i think it is safe.

Wenquan Xing added 2 commits March 9, 2018 15:13
…events for decision will have exactly the same order and no irrelevant event will be inserted in between, so client can predict the event ID of a corresponding decision.
@wxing1292 wxing1292 force-pushed the decision-order-2 branch 2 times, most recently from 7e114ae to ee9a025 Compare March 9, 2018 23:34
Copy link
Contributor

@samarabbas samarabbas left a comment

Choose a reason for hiding this comment

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

Let's hold onto this change until we cut a release.

@wxing1292 wxing1292 merged commit 6164b73 into master Mar 13, 2018
@wxing1292 wxing1292 deleted the decision-order-2 branch March 13, 2018 18:47
wxing1292 pushed a commit that referenced this pull request Mar 13, 2018
wxing1292 pushed a commit that referenced this pull request Mar 13, 2018
wxing1292 added a commit that referenced this pull request Mar 13, 2018
wxing1292 pushed a commit that referenced this pull request Mar 13, 2018
wxing1292 added a commit that referenced this pull request Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants