kaxil commented on code in PR #69869:
URL: https://github.com/apache/airflow/pull/69869#discussion_r3703413738
##########
task-sdk/tests/task_sdk/definitions/test_dag.py:
##########
@@ -447,6 +447,46 @@ def test_continuous_schedule_linmits_max_active_runs(self):
with pytest.raises(ValueError, match="ContinuousTimetable requires
max_active_runs <= 1"):
dag = DAG("continuous", start_date=DEFAULT_DATE,
schedule="@continuous", max_active_runs=25)
+ def
test_timedelta_schedule_respects_create_delta_data_intervals_config(self):
+ """Regression guard: create_delta_data_intervals must control
DeltaTriggerTimetable vs
+ DeltaDataIntervalTimetable for timedelta/relativedelta schedules,
independently of
+ create_cron_data_intervals (which governs only cron-string schedules).
+ """
+ from airflow.sdk.definitions.timetables.interval import
DeltaDataIntervalTimetable
+ from airflow.sdk.definitions.timetables.trigger import
DeltaTriggerTimetable
+
+ from tests_common.test_utils.config import conf_vars
Review Comment:
Can you hoist this to the top of the file? The sibling files here
(`test_connection.py`, `test_variables.py`, `test_operator_resources.py`) all
import `conf_vars` at module level, and there is no circular-import reason for
it to live in the function body.
##########
task-sdk/tests/task_sdk/definitions/test_dag.py:
##########
@@ -447,6 +447,46 @@ def test_continuous_schedule_linmits_max_active_runs(self):
with pytest.raises(ValueError, match="ContinuousTimetable requires
max_active_runs <= 1"):
dag = DAG("continuous", start_date=DEFAULT_DATE,
schedule="@continuous", max_active_runs=25)
+ def
test_timedelta_schedule_respects_create_delta_data_intervals_config(self):
Review Comment:
Consider `@pytest.mark.parametrize` over `(delta, cron, expected)` here: if
the first assert fails, pytest never runs the other two scenarios, and this
file already uses parametrize for similar multi-case checks. A `relativedelta`
row would also back up the docstring, which claims relativedelta coverage while
only `timedelta` is exercised.
##########
task-sdk/src/airflow/sdk/definitions/dag.py:
##########
@@ -147,7 +147,7 @@ def _create_timetable(interval: ScheduleInterval, timezone:
Timezone | FixedTime
if interval == "@continuous":
return ContinuousTimetable()
if isinstance(interval, timedelta | relativedelta):
- if airflow_conf.getboolean("scheduler", "create_cron_data_intervals"):
+ if airflow_conf.getboolean("scheduler", "create_delta_data_intervals"):
Review Comment:
The one-line fix is correct, but it silently changes behavior for the cohort
our own upgrade guide created: `upgrading_to_airflow3.rst` tells 2.x migrants
to set `create_cron_data_intervals = True` to keep data-interval semantics, and
because of this bug that flag currently also pins `timedelta`/`relativedelta`
DAGs to `DeltaDataIntervalTimetable`, which matches Airflow 2 behavior. After
this change those DAGs fall through to `DeltaTriggerTimetable` on the next
parse, so `logical_date` becomes the trigger time and `ds`, `ts` and
`data_interval_*` all shift by one period (no run is skipped in that direction,
but nothing warns the user). The same applies to Airflow 2 serialized DAGs
converted through `conversion_v1_to_v2`, which rebuilds the timetable via this
function.
Can you add a `69869.significant.rst` newsfragment naming who is affected
and the remedy? Setting `[scheduler] create_delta_data_intervals = True` is a
no-op on current main (the key is read nowhere), so users can safely set it
before upgrading to keep current behavior. The docs need the same treatment in
this PR: a bullet in `upgrading_to_airflow3.rst` next to the cron one, this key
in the "Switching between trigger and data interval timetables" section of
`timetable.rst`, and the skip-one-period paragraph the cron entry has in
`config.yml` but the delta entry lacks (flipping this key from False to True
after Airflow 3 runs exist skips one period, same collision guard).
--
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]
