Skip to content
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

Remove timestamp rounding to avoid preliminary timer firing #765

Merged
merged 5 commits into from
Sep 26, 2020

Conversation

mfateev
Copy link
Member

@mfateev mfateev commented Sep 25, 2020

Cassandra timer resolution changed from seconds to milliseconds.

After timer resolution changes this rounding caused preliminary timer firing.

I tested it through Java WorkflowTest.timerTest. It was failing frequently without this fix.

@samarabbas samarabbas merged commit 1214ac0 into temporalio:master Sep 26, 2020
@wxing1292
Copy link
Contributor

@mfateev @samarabbas i suspect these 2 places may be broken due to this change

if isExpired := timerSequence.isExpired(
timestamp.TimeValue(timerTask.VisibilityTime),
timerSequenceID,
); isExpired {

if isExpired := timerSequence.isExpired(
timestamp.TimeValue(timerTask.VisibilityTime),
timerSequenceID,

consider timerTask from above link is generated by timerSequenceID (timestamp being same)
but due to the precision loss, timerTask will be before timerSequenceID causing the isExpired function NOT return true

@wxing1292
Copy link
Contributor

the comparison within the timerQueueActiveTaskExecutor is not strictly speaking correct.

if expired := timerSequence.isExpired(referenceTime, timerSequenceID); !expired {

here the isExpired function is comparing the timerSequence vs "now"

say the original timer task before precision loss (before persistence to DB) is T0+0.999ms, the timer task after the precision loss is T0, now is T0+0.5ms, then the timer task which should be fired will not fire.

@wxing1292
Copy link
Contributor

to solve this

return time.Now().UTC()

and

must be millisecond precision

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants