Remove CronJob "100 missed start times" error (binary search approach) #13
Conversation
vllry
changed the title
vllry
force-pushed
the
cronjobs-100-binarysearch
branch
from
7534e37
to
aa4de58
Compare
vllry
changed the title
vllry
force-pushed
the
cronjobs-100-binarysearch
branch
from
aa4de58
to
b1d481d
Compare
vllry
changed the title
| // If we are looking at a time window less than a minute, there is "room" for at most 1 schedule, and we can skip the binary search. | ||
| minimumTimeUnit := time.Minute | ||
| if endWindow.Sub(startWindow) < minimumTimeUnit { | ||
| nextStart := schedule.Next(startWindow) |
There was a problem hiding this comment.
It would be one less (small) Next call to do more of a check prior to both branches that call getLatestMissedScheduleBinarySearch. When this function calls itself, it already knows if the call will yield a new result or not (duration < minute couldn't possibly contain 2 schedules, so the Next schedule in that window is definitely the last one).
vllry
force-pushed
the
cronjobs-100-binarysearch
branch
from
b1d481d
to
f7716e7
Compare
| } else { | ||
| latestSchedule, found := getLatestMissedScheduleBinarySearch(nextSchedule, endWindow, schedule) | ||
| if found && latestSchedule != nextSchedule { | ||
| return latestSchedule, 2 // We missed at least 2 schedules. Return the latest. |
There was a problem hiding this comment.
I'm a little confused how we conclude that we missed atleast 2 schedules here
There was a problem hiding this comment.
We have 1 schedule (being in the parent else statement implies !nextSchedule.After(endWindow)), AND we have another schedule result after that.
There was a problem hiding this comment.
This structure is a weird hack to get an accurate "none, one, multiple" missed-schedule count (used for telemetry purposes).
There are other ways to get that data, such as returning an (incompte) count from getLatestMissedScheduleBinarySearch, and checking making a Next(startWindow) call if and only if the count is 1. If the count is 1, and there was more than 1 missed start time, Next(startWindow) would return a unique value. If were was really only 1, it would result the same as the final result.
| } | ||
|
|
||
| // Break the window into 2 halves for a binary search. | ||
| midway := startWindow.Add(endWindow.Sub(startWindow) / 2) |
There was a problem hiding this comment.
why not use the standard library call for the binary search implementation?
There was a problem hiding this comment.
There was a problem hiding this comment.
Doing it directly seems more straightforward than trying to cram something into the Search function. It's a binary search approach, but it's not entirely a classical binary search problem.
| if !nextScheduleAfterMidway.After(endWindow) { | ||
| latestFound, found := getLatestMissedScheduleBinarySearch(nextScheduleAfterMidway, endWindow, schedule) // Check a sub-window between the found schedule and the end of the window. | ||
| if found { | ||
| return latestFound, true | ||
| } else { | ||
| return nextScheduleAfterMidway, true | ||
| } | ||
|
|
There was a problem hiding this comment.
Why not just recurse on getLatestMissedScheduleBinarySearch(midway, endWindow, schedule)?
There was a problem hiding this comment.
It's a little more efficient to jump ahead to nextScheduleAfterMidway, and ignore the gap between midway and nextScheduleAfterMidway. The underlying implementation of Next() is sensitive to the duration of time between the start and the result - for example, Next() is a bit slower to get the next monthly schedule than the next minutely one.
vllry
force-pushed
the
cronjobs-100-binarysearch
branch
from
fe8217f
to
87476dc
Compare
Co-Authored-By: Tom Wanielista <tomwans@users.noreply.github.com> Co-Authored-By: Kevin Yang <16491459+yangkev@users.noreply.github.com>
vllry
force-pushed
the
cronjobs-100-binarysearch
branch
from
87476dc
to
8410402
Compare
This PR is a less efficient but simpler alternative to #12
As the robfig/cron module only exposes a Next() function for schedules, this PR uses a binary search, rather than brute force, to traverse the schedule window.
The flow of
getLatestMissedScheduleBinarySearchis roughly this:The answer is the latest missed start time that is found.
getRecentUnmetScheduleTimes()is still the interface for performing this check (in the interest of easy patching for the next few releases).getRecentUnmetScheduleTimes()retains the required properties of its return types - specifically,results[-1]is the most result missed start time, and0 <= len(results) <= 2` (used for events/logging). The results no longer contain a precise count, or other start times prior to the most recent one.