kaxil commented on code in PR #69869:
URL: https://github.com/apache/airflow/pull/69869#discussion_r4401074004
##########
airflow-core/newsfragments/69869.significant.rst:
##########
@@ -0,0 +1,63 @@
+Fix ``create_delta_data_intervals`` being ignored for timedelta/relativedelta
schedules
Review Comment:
Could the title say what users see rather than "Fix ..."? Something like
"``[scheduler] create_delta_data_intervals`` now controls ``timedelta`` and
``relativedelta`` schedules". A few spots in the body have the same problem
("Before this fix" on line 4, the present-tense "are also unintentionally
running" on line 12), which read wrong in the notes for the release that ships
it.
##########
airflow-core/newsfragments/69869.significant.rst:
##########
@@ -0,0 +1,63 @@
+Fix ``create_delta_data_intervals`` being ignored for timedelta/relativedelta
schedules
+
+``[scheduler] create_delta_data_intervals`` now correctly controls timetable
selection
+for DAGs that pass a ``timedelta`` or ``relativedelta`` to ``schedule=``.
Before this
+fix, ``create_cron_data_intervals`` silently governed both cron-string DAGs
**and**
+timedelta/relativedelta DAGs, making ``create_delta_data_intervals`` a no-op.
+
+**Who is affected:**
+
+Users who set ``create_cron_data_intervals = True`` in their Airflow 3 config
to
+preserve Airflow 2 data-interval semantics for cron DAGs (as recommended by
+``upgrading_to_airflow3.rst``) are also unintentionally running their
+``timedelta``/``relativedelta`` DAGs on ``DeltaDataIntervalTimetable``. After
this
+fix, those DAGs will fall through to the correct default
(``DeltaTriggerTimetable``)
+unless ``create_delta_data_intervals`` is also set to ``True``.
+
+This also affects DAGs migrated from Airflow 2 via ``conversion_v1_to_v2``,
which
+rebuilds timetables through the same code path.
+
+**Behaviour changes:**
+
+- ``create_delta_data_intervals`` now takes effect as documented.
+- ``create_cron_data_intervals`` now affects **only** cron-string DAGs.
+- The two config keys are independent, as ``config.yml`` always documented.
+
+**Migration:**
+
+If you rely on ``DeltaDataIntervalTimetable`` for
``timedelta``/``relativedelta``
+DAGs (i.e. you need contiguous data intervals and ``ds``/``ts`` anchored at
+``data_interval_start``), set this **before** the next scheduler restart::
+
+ [scheduler]
+ create_delta_data_intervals = True
+
+You can safely set this flag on current ``main`` (before this fix ships)
because
Review Comment:
This misses the other group whose timetable changes on upgrade: anyone with
`create_cron_data_intervals = False` and `create_delta_data_intervals = True`.
Today they get `DeltaTriggerTimetable`, after this they get
`DeltaDataIntervalTimetable`, and the collision guard in
`_DataIntervalTimetable.next_dagrun_info` skips one period. So "no risk of an
unexpected timetable switch" only holds when the cron key is also `True`. Can
you add them under "Who is affected"?
This paragraph also reads oddly once released ("current `main`", "until the
fix is in place"). Something like "The setting has no effect on releases that
ignore it, so it can be set before upgrading" ages better. Same for line 30:
the timetable is picked when the Dag processor parses the file, not at
scheduler restart, so "before upgrading" is the right moment there too.
##########
airflow-core/docs/authoring-and-scheduling/timetable.rst:
##########
@@ -368,10 +368,14 @@ scheduled run, because the next run is advanced one
period to avoid colliding
with the previous run's ``logical_date``. The reverse direction (data
interval -> trigger) does not skip a run.
-This transition can happen without editing a Dag, in two ways:
+This transition can happen without editing a Dag, in three ways:
Review Comment:
This file merges cleanly onto main, so the rebase will keep the `.. note::`
that #70658 added under "Differences between trigger and data interval
timetables" (saying `create_delta_data_intervals` "is not currently
consulted"), plus the sentence just before it that tells timedelta users to set
`create_cron_data_intervals` to get contiguous windows. Both are wrong once
this lands. Can you drop the note and have that paragraph name each key for its
own schedule type? `core-concepts/dag-run.rst` already describes the fixed
behaviour, so it can stay. With three bullets here, "across either change"
below should become "any of these changes" too.
##########
airflow-core/newsfragments/69869.significant.rst:
##########
@@ -0,0 +1,63 @@
+Fix ``create_delta_data_intervals`` being ignored for timedelta/relativedelta
schedules
+
+``[scheduler] create_delta_data_intervals`` now correctly controls timetable
selection
+for DAGs that pass a ``timedelta`` or ``relativedelta`` to ``schedule=``.
Before this
+fix, ``create_cron_data_intervals`` silently governed both cron-string DAGs
**and**
+timedelta/relativedelta DAGs, making ``create_delta_data_intervals`` a no-op.
+
+**Who is affected:**
+
+Users who set ``create_cron_data_intervals = True`` in their Airflow 3 config
to
+preserve Airflow 2 data-interval semantics for cron DAGs (as recommended by
+``upgrading_to_airflow3.rst``) are also unintentionally running their
+``timedelta``/``relativedelta`` DAGs on ``DeltaDataIntervalTimetable``. After
this
+fix, those DAGs will fall through to the correct default
(``DeltaTriggerTimetable``)
+unless ``create_delta_data_intervals`` is also set to ``True``.
+
+This also affects DAGs migrated from Airflow 2 via ``conversion_v1_to_v2``,
which
+rebuilds timetables through the same code path.
+
+**Behaviour changes:**
+
+- ``create_delta_data_intervals`` now takes effect as documented.
+- ``create_cron_data_intervals`` now affects **only** cron-string DAGs.
+- The two config keys are independent, as ``config.yml`` always documented.
+
+**Migration:**
+
+If you rely on ``DeltaDataIntervalTimetable`` for
``timedelta``/``relativedelta``
+DAGs (i.e. you need contiguous data intervals and ``ds``/``ts`` anchored at
+``data_interval_start``), set this **before** the next scheduler restart::
+
+ [scheduler]
+ create_delta_data_intervals = True
+
+You can safely set this flag on current ``main`` (before this fix ships)
because
+``create_delta_data_intervals`` was previously a no-op - setting it to ``True``
+will have no effect until the fix is in place, so there is no risk of an
+unexpected timetable switch during the transition.
+
+Switching a DAG from ``DeltaTriggerTimetable`` to
``DeltaDataIntervalTimetable``
+(i.e. flipping ``create_delta_data_intervals`` from ``False`` to ``True`` when
+existing DAG runs are present) skips one scheduled run to avoid colliding with
+the previous run's ``logical_date``. Plan for this one-period gap or convert
+affected DAGs to an explicit ``DeltaDataIntervalTimetable(...)`` instance in
Review Comment:
For a Dag that is on `DeltaTriggerTimetable` today, switching to an explicit
`DeltaDataIntervalTimetable(...)` hits the same guard and skips the same run,
so this doesn't avoid the gap. It only works for Dags already on the
data-interval timetable.
##########
airflow-core/docs/installation/upgrading_to_airflow3.rst:
##########
@@ -388,6 +388,26 @@ These include:
Airflow 3 dagruns already exist (going
``CronTriggerTimetable`` -> ``CronDataIntervalTimetable``), one scheduled run
is skipped to avoid colliding with the previous run's ``logical_date``.
+
+- The ``create_delta_data_intervals`` configuration is now **functional**.
Previously, a bug caused
+ ``create_cron_data_intervals`` to silently control timetable selection for
both cron-string DAGs
+ **and** ``timedelta``/``relativedelta`` DAGs, making
``create_delta_data_intervals`` a no-op.
+
+ This affects users who set ``create_cron_data_intervals = True`` to preserve
Airflow 2
+ data-interval semantics for cron DAGs (as the upgrade guide recommends).
Because of the bug,
+ that flag was also keeping ``timedelta``/``relativedelta`` DAGs on
``DeltaDataIntervalTimetable``.
+ After this fix those DAGs fall through to ``DeltaTriggerTimetable`` instead,
shifting
Review Comment:
I had this wrong in my earlier comment: `data_interval_end` doesn't move.
`DeltaTriggerTimetable` defaults to `interval=timedelta()`, so a run's window
goes from `[t - delta, t]` to `[t, t]`. `logical_date`, `ds`, `ts` and
`data_interval_start` shift by one period and the window collapses to zero
width, so a query between start and end reads nothing. Also,
`conversion_v1_to_v2` is a private name users can't act on (here and on line 17
of the newsfragment); "Dags whose serialized form was written by Airflow 2"
says the same thing.
##########
airflow-core/docs/installation/upgrading_to_airflow3.rst:
##########
@@ -388,6 +388,26 @@ These include:
Airflow 3 dagruns already exist (going
``CronTriggerTimetable`` -> ``CronDataIntervalTimetable``), one scheduled run
is skipped to avoid colliding with the previous run's ``logical_date``.
+
+- The ``create_delta_data_intervals`` configuration is now **functional**.
Previously, a bug caused
Review Comment:
People coming from 2.x will read this long after it ships, so "now
**functional**", "Previously, a bug", "After this fix" and "current unpatched
Airflow 3" won't mean much to them. What they need is the fact this guide is
missing today: Airflow 2 always used `DeltaDataIntervalTimetable` for
timedelta/relativedelta schedules, and Airflow 3 defaults
`create_delta_data_intervals` to `False`, so those Dags move to
`DeltaTriggerTimetable` unless it's set to `True` before upgrading. The bug
history fits better in the newsfragment.
--
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]
