Make the timezone aware triggers resilient to delays #86
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
The cron processing includes doing the work of each scheduled job inline at the point they detect they need to run. This works well for the jobs where the scheduling is entirely handled by the scheduler, because they set a next time to run and are then run at some point after that time occurs. However for the timezone aware triggers we're somewhat abusing that mechanism to do our own timezone awareness filtering. As a result, our reliance on being run at exactly (within a minute of) the right time meant that the triggers were missing a significant proportion of the actual times at which they should have run their processing. This change makes the triggers stateful -- aware of the last real time at which they ran and thus able to account for those delays. They now check all the minutes between when they last ran and the current time, thus ensuring that delays still result in the trigger running. There is a side effect of this approach -- that delays may cause a trigger to only run once where it might ideally run several times. This is a worthwhile trade-off for now and is certainly better than not running at all.
danpalmer
approved these changes
Sep 2, 2019
Use a timezone aware range comparison rather than evaluating every minute within the range. This should be faster while still being correct.
This copies the same time-range approach used to optimise the TimezoneAwareProcessor into the MetadataTimezoneAwareProcessor. The performance increase here is less stark (I suspect because iterating over all the timezones dominates), however this is likely to still be an improvement. In any case it's beneficial to have both processors working in the same way.
danpalmer
approved these changes
Sep 2, 2019
It's not really possible for the processor to run twice at the same instant, so change the tests not to do that.
Internal errors would break the overall cron processing, so we want to avoid that.
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.
The cron processing includes doing the work of each scheduled job inline at the point they detect they need to run. This works well for the jobs where the scheduling is entirely handled by the scheduler, because they set a next time to run and are then run at some point after that time occurs.
However for the timezone aware triggers we're somewhat abusing that mechanism to do our own timezone awareness filtering. As a result, our reliance on being run at exactly (within a minute of)
the right time meant that the triggers were missing a significant proportion of the actual times at which they should have run their processing.
This change makes the triggers stateful -- aware of the last real time at which they ran and thus able to account for those delays. They now check all the minutes between when they last ran and the current time, thus ensuring that delays still result in the trigger running.
There is a side effect of this approach -- that delays may cause a trigger to only run once where it might ideally run several times. This is a worthwhile trade-off for now and is certainly better than not running at all.