kaxil commented on PR #70658: URL: https://github.com/apache/airflow/pull/70658#issuecomment-5942462466
The examples match what the scheduler does now, LGTM. I have two wording nits you can take or leave: 1. The no-`start_date` sentence (lines 387-389) is too broad, and that's on me: my earlier comment said the default `run_immediately` picks the next future tick, which only holds for the example's 3PM. `CronTriggerTimetable._calc_first_run` special-cases only `True` and a `timedelta`, so `False` falls into the same buffer as `None` and still runs the previous tick when it is less than `max(period / 10, 5 min)` old (`test_run_immediately` pins `(False, JUST_AFTER, PREVIOUS)`). An `@daily` Dag enabled at 00:30 gets the midnight run immediately. `DeltaTriggerTimetable` has no `run_immediately` at all and creates its first run at pickup time. Scoping the sentence to `CronTriggerTimetable` and mentioning the tolerance would cover it. 2. The bullet at lines 335-337 says `logical_date` and the `run_id` timestamp differ between the two kinds, and line 386 says they differ in how `run_id` is derived. Both kinds take the `run_id` timestamp from `run_after` (`Timetable.generate_run_id`), and your own first example shows it: both runs get a midnight-January-31st `run_id`. I'd drop `run_id` from both places and keep `logical_date` and the data interval. Separately, the existing table under "Differences between the cron and delta data interval timetables" now contradicts the new catchup text. For `timedelta(minutes=30)` with `catchup=False` enabled at 01:05, `DeltaDataIntervalTimetable._skip_to_latest` floors to the 30-minute grid and covers 00:30-01:00, not 00:35-01:05, for both start dates. Fine as a follow-up. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
