U/krall/croniter - #1108
Conversation
…obs throughout the hour or day.
| name="cron", | ||
| original=config.original, | ||
| jitter=config.jitter, | ||
| hash_id=job_name, |
There was a problem hiding this comment.
If we end up using the Jenkins-style hashed expressions in any capacity I think equivalent Airflow support should be a prerequisite (or...postrequisite?).
They don't support the H syntax, so either we'd need to implement something like this that returns an Airflow CronTriggerTimetable or Airflow folks would need to implement something vaguely similar (coincidentally, there is a recent PR up to add JitteredCronTimetable, though that wouldn't be 1:1 in it's current approach).
| else: | ||
| start_time = start_time.astimezone(self.time_zone) | ||
|
|
||
| it = croniter(self.cron_expression, start_time, hash_id=self.hash_id) |
There was a problem hiding this comment.
I think replacing self.time_spec.get_match(start_time) with a direct call to croniter changes our documented DST behaviour (for tz aware jobs that include a run in the transition hours).
The GeneralScheduler calculates the next match as naive local time and then localizes it in trontimespec. Right now it catches ambiguous times and selects the first occurrence with is_dst=True (sg).
Passing a tz aware time directly to croniter lets it hit both instances, so 30 1 * * * will schedule both 1:30 PDT and 1:30 PST. I think we want croniter to calculate against naive local time, then apply Tron’s existing timezone policy.
There was a problem hiding this comment.
Oh, we have test_fall_back, but that just looks at generalscheduler
|
in light of #1072 being closed, I might want to add a test for Feb 30 |
| ) | ||
| except ValueError as e: | ||
| expression = re.sub(r"\s*,\s*", ",", config.value.strip()) | ||
| if not croniter.is_valid(expression, hash_id="validation_placeholder"): |
There was a problem hiding this comment.
Should this use the actual hash_id instead of a placeholder? If we have someone doing 0 0 H 2 * we could end up with 30/31 since Croniter hashes H across the full 1–31 range. A less likely but funnier risk would be 0 0 31 H *
| with pytest.raises(CroniterBadDateError): | ||
| sched.next_run_time(start_time) | ||
|
|
||
|
|
There was a problem hiding this comment.
Thoughts on repurposing some of these GeneralScheduler tests for CronScheduler?
I think some of these should be carried forward. E.g.
test_fall_backshould look at CronScheduler("30 1 * * *")test_spring_forwardshould look at CronScheduler("30 2 * * *") to ensure we shift nonexistent times forwardtest_handles_unsetting_the_time_zoneandtest_handles_changing_the_time_zoneboth test behaviour we should maintain
There was a problem hiding this comment.
Jeez, test_handles_unsetting_the_time_zone is just incorrect in its current form. I like the spirit of it, but imo it shouldn't start with UTC and it shouldn't assert hour == 0.
There was a problem hiding this comment.
JEEZ, test_handles_changing_the_time_zone is also funky. I don't think this should look at datetime.now and I very much disagree with asserting on just the hour.
3 participants
This should be backwards-compatible, as we're already validating cron expressions in yelpsoa-configs using croniter. This PR provides a few benefits:
mon#2or1#2), so once we ship this, we can migrate anything the groc syntax to croniter, then delete even more code.