TS/runtime-5: align virtual/real time in all types of scheduled task category#10305
Conversation
feiyang3cat
changed the title
feiyang3cat
force-pushed
the
ts/patch-5
branch
3 times, most recently
from
3d50bdf to
a3f1761
Compare
feiyang3cat
changed the title
feiyang3cat
force-pushed
the
ts/patch-5
branch
2 times, most recently
from
4ab596d to
cca239e
Compare
feiyang3cat
changed the title
feiyang3cat
changed the title
feiyang3cat
changed the title
feiyang3cat
force-pushed
the
ts/patch-5
branch
6 times, most recently
from
8c53840 to
d848ebf
Compare
| // real wall-clock, so convert here — callers outside MutableState don't see the virtual | ||
| // vs. real distinction. The CategoryTypeScheduled drop-check above runs first so it | ||
| // compares virtual-vs-virtual (now is also virtual). | ||
| if skip > 0 && category == tasks.CategoryTimer { |
There was a problem hiding this comment.
though we add other scheduled timer task types in, the executors of archival and speculative tasks don't validate the fire time, so for those tasks the executor files are untouched
feiyang3cat
force-pushed
the
ts/patch-5
branch
2 times, most recently
from
7a2bf1f to
f0690d9
Compare
| // compares virtual-vs-virtual (now is also virtual). | ||
| if skip > 0 && category == tasks.CategoryTimer { | ||
| if skip > 0 && category.Type() == tasks.CategoryTypeScheduled { | ||
| task.SetVisibilityTime(task.GetVisibilityTime().Add(-skip)) |
There was a problem hiding this comment.
nit: could you also use ms.ToRealTime here? might also be able to fold fast return when skip == 0 case into ToRealTime
| return nil, serviceerror.NewInternal(errString) | ||
| } | ||
|
|
||
| if queues.IsTimeExpired( |
There was a problem hiding this comment.
this function uses wall time in task.GetKey().FireTime, but virtual time for rest of parameters
There was a problem hiding this comment.
oh, I saw the link you pasted below about the origin of this function. I think I misunderstood the use of this function, and have changed all the three time points to real time. pls have another look. thanks !
| return nil, serviceerror.NewInternal(errString) | ||
| } | ||
|
|
||
| if queues.IsTimeExpired( |
| ctx context.Context, | ||
| timerTask *tasks.UserTimerTask, | ||
| ) error { | ||
| referenceTime := t.Now() |
There was a problem hiding this comment.
@yycptt, any reason why you set this here, rather than inside action? Reading through https://github.com/temporalio/temporal/pull/7030/changes, seems like it is fine to do it inside.
There was a problem hiding this comment.
(btw: there is no need to change this line after I switched to real time for comparison in task queues
feiyang3cat
force-pushed
the
ts/patch-5
branch
5 times, most recently
from
3b060f0 to
0f7227d
Compare
feiyang3cat
force-pushed
the
ts/patch-5
branch
8 times, most recently
from
35ce9d8 to
bb473c3
Compare
| // AddTasks adds tasks to the mutable state. | ||
| // For scheduled tasks (any CategoryTypeScheduled — e.g. timer, archival), if time has been | ||
| // skipped (i.e. the virtual time of this mutable state is ahead of wall-clock time), the | ||
| // scheduled time of the task is adjusted to wall-clock time, as the dispatch queues run |
There was a problem hiding this comment.
nit: for clarity, it is worth to specify that the virtual time should be passed in, but it will be adjusted to the wall-clock
2 participants
What changed?
uses ms.Now() (virtual) instead of wall.
ToRealTime(heartbeatTimeoutVis) (was mixed frames), (2) for activity timeout/user timer time check, (3) for run time our retry, inherit time skipping config
Now, SetSpeculativeWorkflowTaskTimeoutTask.
Why?
Under time skipping, virtual time inside mutable state can run ahead of wall time, but scheduled-task
firing still runs on wall time. Each crossing needs explicit conversion:
Before this change, virtual and wall values were silently compared — harmless pre-skipping, but now lets
the wrong tasks get dropped/kept and stale heartbeat tasks pass dedup.
How did you test it?