-
Notifications
You must be signed in to change notification settings - Fork 805
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
Implement the functionality to just wait for the last event when dumping history events #470
Conversation
20: optional string runId | ||
30: optional shared.TaskList tasklist | ||
40: optional bool isWorkflowRunning | ||
// the event ID in the history event table which represent last batch of events | ||
50: optional i64 (js.type = "Long") lastFirstEventId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a good time to rename this API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already done in #464
|
||
// It is possible that we still have the events in the table even though the mutable state is gone | ||
// Get the nextEventID from visibility store if we still have it. | ||
visibilityResp, err := wh.visibitiltyMgr.GetClosedWorkflowExecution(&persistence.GetClosedWorkflowExecutionRequest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleanup the code path where we read/write NextEventID to closed workflow execution visibility store after completion. Do make sure it is not getting used anywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
searching using vs code
the call GetClosedWorkflowExecution only appears in:
VisibilityManager.go
cassandraVisibilityPersistence_test.go
cassandraVisibilityPersistence.go
visibilityInterfaces.go
so i think we are good
return nil, wh.error(err, scope) | ||
} | ||
// since getHistory func will not return empty history, so the below is safe | ||
history.Events = history.Events[len(history.Events)-1 : len(history.Events)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you validate there is atleast one event returned by GetHistory?
Emit an error log incase no event is returned back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if no events return (or empty), persistence will return err
service/history/historyEngine.go
Outdated
@@ -285,6 +285,7 @@ func (e *historyEngineImpl) StartWorkflowExecution(startRequest *h.StartWorkflow | |||
if err1 != nil { | |||
return nil, err1 | |||
} | |||
msBuilder.executionInfo.LastFirstEventID = *startedEvent.EventId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the generated Getter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
history.Events = events | ||
if !token.IsWorkflowRunning { | ||
history.Events = []*gen.HistoryEvent{} | ||
if isCloseEventOnly { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it makes sense to push the filter all the way down to history service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done using the int64 max as expected next event ID
@@ -127,6 +127,7 @@ type ( | |||
ExecutionContext []byte | |||
State int | |||
CloseStatus int | |||
LastFirstEventID int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need persistence changes to read/write this new field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ec6264b
to
b4b1fc0
Compare
b4b1fc0
to
d80a4d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One minor comment. You can check it in after fixing it.
common/persistence/dataInterfaces.go
Outdated
@@ -360,6 +360,7 @@ type ( | |||
WorkflowTimeout int32 | |||
DecisionTimeoutValue int32 | |||
ExecutionContext []byte | |||
LastFirstEventID int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just define this as a constant? This will always have a fixed value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cba2a41
to
8656c4f
Compare
…463) * implement custpomized deduplication of start workflow execution API
…vent of when dumping history events
add missing persistant layer change
8656c4f
to
f73c6f2
Compare
…vent of when dumping history events