-
Notifications
You must be signed in to change notification settings - Fork 825
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
Timeout workflow when WorkflowExecutionTimeout is reached #560
Conversation
common/backoff/cron.go
Outdated
@@ -58,8 +58,14 @@ func GetBackoffForNextSchedule(cronSchedule string, startTime time.Time, closeTi | |||
if err != nil { | |||
return NoBackoff | |||
} | |||
|
|||
if closeTime.Before(startTime) { |
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.
I don't believe this is the right fix. If startTime comes after closeTime, then it means some other event triggered invocation of this code path like workflow timeout. In this case we should still try to fire the cron on previous value so we should just return start the delta between startTime and closeTime immediately.
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.
Changes in timerQueueActiveTaskExecutor.go looks good. But you need to slightly adjust the logic in GetBackoffNextSchedule based on the comment I left.
0abb1c2
to
e1b6c44
Compare
What changed?
If
WorkflowExecutionTimeout
is reached on workflow, workflow is timed out in spite of cron schedule.Also fix
GetBackoffForNextSchedule
to return right result ifcloseTime
is beforestartTime
.Why?
Fixing bugs discovered in #394.
How did you test it?
Added new unit tests.
Potential risks
No risks.