Skip to content

Check next run exists before reading data interval - #19307

Merged
kaxil merged 2 commits into
apache:mainfrom
uranusjr:aip-39-next-data-interval-must-check-non-none-schedule
Nov 1, 2021
Merged

Check next run exists before reading data interval#19307
kaxil merged 2 commits into
apache:mainfrom
uranusjr:aip-39-next-data-interval-must-check-non-none-schedule

Conversation

@uranusjr

@uranusjr uranusjr commented Oct 29, 2021

Copy link
Copy Markdown
Member

Fix #19304, and also an issue on scheduling a DAG's first-ever run introduced in #18897. We could fix it outside this function, but if next_dagrun is None, the next run's data interval is supposed to be None in the first place, so checking inside this function just makes sense.

closes #19343
closes #19304

@uranusjr
uranusjr force-pushed the aip-39-next-data-interval-must-check-non-none-schedule branch from 400376c to 5761ee3 Compare October 30, 2021 03:03
@uranusjr
uranusjr marked this pull request as ready for review October 30, 2021 03:11
Comment thread tests/models/test_dag.py Outdated
pytest.param(
DEFAULT_DATE,
DEFAULT_DATE,
DEFAULT_DATE + timedelta(days=1),

@dstandish dstandish Oct 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the modern case, your explicit data interval and the schedule interval are both 1 day. so it's sortof ambiguous what's driving the result (i.e. is it cus of the data interval you provided, or because of the schedule interval?). seems it might be better to choose an interval that differs from the schedule interval of the dag, to make it clearer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. It is logically not possible for a DAG to return different data intervals via inference and explicit timetable (for obvious reasons), but it is a good way to test the behaviour in a unit test.

@kaxil
kaxil requested a review from dstandish November 1, 2021 18:57
@github-actions github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 1, 2021
@github-actions

github-actions Bot commented Nov 1, 2021

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@kaxil
kaxil merged commit dc4dcaa into apache:main Nov 1, 2021
jedcunningham pushed a commit to astronomer/airflow that referenced this pull request Nov 1, 2021
)

Fix apache#19304, and also an issue on scheduling a DAG's first-ever run introduced in apache#18897. We could fix it outside this function, but if `next_dagrun` is None, the next run's data interval is supposed to be None in the first place, so checking inside this function just makes sense.

closes apache#19343
closes apache#19304

(cherry picked from commit dc4dcaa)
jedcunningham pushed a commit to astronomer/airflow that referenced this pull request Nov 1, 2021
)

Fix apache#19304, and also an issue on scheduling a DAG's first-ever run introduced in apache#18897. We could fix it outside this function, but if `next_dagrun` is None, the next run's data interval is supposed to be None in the first place, so checking inside this function just makes sense.

closes apache#19343
closes apache#19304

(cherry picked from commit dc4dcaa)
@uranusjr
uranusjr deleted the aip-39-next-data-interval-must-check-non-none-schedule branch November 2, 2021 07:30
kaxil pushed a commit that referenced this pull request Nov 2, 2021
Fix #19304, and also an issue on scheduling a DAG's first-ever run introduced in #18897. We could fix it outside this function, but if `next_dagrun` is None, the next run's data interval is supposed to be None in the first place, so checking inside this function just makes sense.

closes #19343
closes #19304

(cherry picked from commit dc4dcaa)
jedcunningham pushed a commit that referenced this pull request Nov 3, 2021
Fix #19304, and also an issue on scheduling a DAG's first-ever run introduced in #18897. We could fix it outside this function, but if `next_dagrun` is None, the next run's data interval is supposed to be None in the first place, so checking inside this function just makes sense.

closes #19343
closes #19304

(cherry picked from commit dc4dcaa)
@en1cc

en1cc commented Nov 15, 2021

Copy link
Copy Markdown

Is there a possible window of time when this is going to be released? We have the same problem in our Environment and discuss if we downgrade or wait for this fix.

@uranusjr

Copy link
Copy Markdown
Member Author

No hard timeline, it all depends on how the release can be properly tested and vetted. See #19558 for latest update.

@kaxil

kaxil commented Nov 15, 2021

Copy link
Copy Markdown
Member

Airflow 2.2.2 has now been released that contains this fix. @en1cc

@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full tests needed We need to run full set of tests for this PR to merge type:bug-fix Changelog: Bug Fixes

Projects

None yet

5 participants