-
Notifications
You must be signed in to change notification settings - Fork 863
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
Handle system time move backward case in timer task processing #7030
base: main
Are you sure you want to change the base?
Conversation
fb5f5fd
to
c3f87c1
Compare
@@ -303,10 +304,14 @@ func (p *scheduledQueue) lookAheadTask() { | |||
// IsTimeExpired checks if the testing time is equal or before | |||
// the reference time. The precision of the comparison is millisecond. | |||
func IsTimeExpired( |
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 would add a note explaining why we take the task here (to prevent clock skew issues).
// NOTE: Persistence layer may lose precision when persisting the task, which essentially moves | ||
// task fire time backward. But we are already performing truncation here, so doesn't need to | ||
// account for that. | ||
referenceTime = util.MaxTime(referenceTime, task.GetKey().FireTime).Truncate(persistence.ScheduledTaskMinPrecision) |
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.
What's the difference between visibility timestamp and fire time and why take that here?
@@ -236,7 +240,7 @@ func (t *timerQueueStandbyTaskExecutor) executeActivityTimeoutTask( | |||
// created. | |||
isHeartBeatTask := timerTask.TimeoutType == enumspb.TIMEOUT_TYPE_HEARTBEAT | |||
ai, heartbeatTimeoutVis, ok := mutableState.GetActivityInfoWithTimerHeartbeat(timerTask.EventID) | |||
if isHeartBeatTask && ok && queues.IsTimeExpired(timerTask.GetVisibilityTime(), heartbeatTimeoutVis) { | |||
if isHeartBeatTask && ok && queues.IsTimeExpired(timerTask, timerTask.GetVisibilityTime(), heartbeatTimeoutVis) { |
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.
Shouldn't we use referenceTime
to allow progressing beyond the visibility timestamp in case task processing is delayed for a while?
What changed?
Why?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?