Check for resurrected activities during RecordActivityTaskStarted #4806
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 0180d1f5-182c-4cdd-835e-03a89a32cbdb
💛 - Coveralls |
davidporter-id-au
approved these changes
May 16, 2022
| if err != nil { | ||
| return nil, err | ||
| } | ||
| event := item.(*types.HistoryEvent) |
There was a problem hiding this comment.
nit, maybe typecheck this is ok?
There was a problem hiding this comment.
This code was moved (from service/history/task/timer_active_task_executor.go) as is. Going to leave it for now. Ideally we should introduce generics here with go 1.18.
| // RecordActivityTaskStarted is already past scheduleToClose timeout. | ||
| // If at this point pending activity is still in mutable state it may be resurrected. | ||
| // Otherwise it would be completed or timed out already. | ||
| if isRunning && e.timeSource.Now().After(ai.ScheduledTime.Add(time.Duration(ai.ScheduleToCloseTimeout)*time.Second)) { |
There was a problem hiding this comment.
What's the risk of this being a false-positive due to clock-skew or GC or something?
There was a problem hiding this comment.
What's the risk of this being a false-positive due to clock-skew or GC or something?
This is just a trigger check. We don't want to run those checks often as they are potentially expensive with large histories. Worst case - wasted resources, increased latencies.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
historyEngine.RecordActivityTaskStartedif it is already past scheduleToClose timeout.Why?
Similarly to timer task processing, resurrected activity can sometimes be observed when matching triggers RecordActivityTaskStarted. In those cases activity gets started twice which result in non-deterministic errors, failures to replicate to remote cluster followed by DLQ messages.
To mitigate that we could reuse logic used in timer task processing and scan for previous history if such activity was already completed before. If so, delete it from mutable state and return that it does not exist.
As history scan may be expensive - only do it if timing of RecordActivityTaskStarted is suspicious. That is - if it is attempted to start after it should already be completed or timed out.
How did you test it?
Added additional unit test for history engine simulating resurrected activity condition.
Potential risks
Release notes
Documentation Changes