Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed backfill interference with scheduler #22701

Merged
merged 1 commit into from Apr 8, 2022
Merged

Fixed backfill interference with scheduler #22701

merged 1 commit into from Apr 8, 2022

Conversation

houqp
Copy link
Member

@houqp houqp commented Apr 1, 2022

The error message included in the commit should be fairly self-explanatory :)

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:Scheduler Scheduler or dag parsing Issues label Apr 1, 2022
@houqp houqp force-pushed the upstream_backfill_interference branch from 7a00543 to 1c924bf Compare April 2, 2022 04:16
@houqp
Copy link
Member Author

houqp commented Apr 2, 2022

cc @dimonchik-suvorov

@houqp houqp force-pushed the upstream_backfill_interference branch 2 times, most recently from a39f51a to a2e6705 Compare April 2, 2022 17:47
@potiuk
Copy link
Member

potiuk commented Apr 4, 2022

Why do you change states to NONE ? Were the tests wrong to create dagruns in RUNNING state in those tests.

Can we also have a backfill test covering the warning?

@houqp
Copy link
Member Author

houqp commented Apr 4, 2022

Why do you change states to NONE ? Were the tests wrong to create dagruns in RUNNING state in those tests.

Yeah, we think it's a bug to allow running backfill against an active dag run managed by the scheduler. This was allowed in the current airflow main branch and this PR prevents it.

I will add a test for it today 👍

@houqp houqp force-pushed the upstream_backfill_interference branch from a2e6705 to ed091f8 Compare April 4, 2022 23:16
@houqp
Copy link
Member Author

houqp commented Apr 4, 2022

@potiuk test added.

@houqp houqp requested a review from potiuk April 4, 2022 23:17
@houqp houqp force-pushed the upstream_backfill_interference branch 2 times, most recently from 5bf4025 to 04732c1 Compare April 5, 2022 00:11
potiuk
potiuk approved these changes Apr 5, 2022
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Looks cool @houqp ! - just one NIT about asserting error message.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Apr 5, 2022
@github-actions
Copy link

github-actions bot commented Apr 5, 2022

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.

@houqp houqp force-pushed the upstream_backfill_interference branch from 04732c1 to f51708e Compare April 6, 2022 00:30
@houqp houqp closed this Apr 6, 2022
@houqp houqp reopened this Apr 6, 2022
@houqp houqp force-pushed the upstream_backfill_interference branch 2 times, most recently from d8bd9ba to a020b3f Compare April 6, 2022 01:55
uranusjr
uranusjr reviewed Apr 6, 2022
View reviewed changes
airflow/jobs/backfill_job.py Outdated Show resolved Hide resolved
airflow/jobs/backfill_job.py Outdated Show resolved Hide resolved
@houqp houqp force-pushed the upstream_backfill_interference branch 2 times, most recently from 0b6e5dd to 56b29c0 Compare April 6, 2022 18:33
@houqp houqp requested a review from uranusjr April 6, 2022 18:36
@houqp houqp force-pushed the upstream_backfill_interference branch 2 times, most recently from 7940588 to f5f788e Compare April 8, 2022 01:50
@houqp houqp force-pushed the upstream_backfill_interference branch 3 times, most recently from 2945b03 to ee0563c Compare April 8, 2022 06:15
uranusjr
uranusjr reviewed Apr 8, 2022
View reviewed changes
tests/conftest.py Outdated Show resolved Hide resolved
uranusjr
uranusjr reviewed Apr 8, 2022
View reviewed changes
tests/conftest.py Outdated Show resolved Hide resolved
uranusjr
uranusjr reviewed Apr 8, 2022
View reviewed changes
tests/jobs/test_backfill_job.py Outdated
start_date=DEFAULT_DATE,
end_date=DEFAULT_DATE + datetime.timedelta(days=2),
)
with patch.object(job.log, 'error') as mock_error:
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to mock this, you can use pytest’s caplog fixture.

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL, thanks for the tip!

@uranusjr
Copy link
Member

uranusjr commented Apr 8, 2022

A few minor comments on implementation, the general logic looks right to me.

@houqp houqp force-pushed the upstream_backfill_interference branch from ee0563c to 6d5e283 Compare April 8, 2022 18:22
@houqp houqp requested a review from uranusjr April 8, 2022 18:23
ashb
ashb reviewed Apr 8, 2022
View reviewed changes
tests/jobs/test_scheduler_job.py
@@ -1760,7 +1760,7 @@ def evaluate_dagrun(
dr = dag.create_dagrun(
run_type=DagRunType.SCHEDULED,
execution_date=dagrun_info.logical_date,
state=State.RUNNING,
Copy link
Member

Choose a reason for hiding this comment

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

Why did you need to change these tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

because these tests created a scheduled dagrun in running state and this fix prevents a backfill run from overriding a scheduled DAG run.

Copy link
Member

Choose a reason for hiding this comment

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

Ohh, yes, the scheduler tests that use dag.run which actually uses BackfillJob. We should fix that at some point.

@houqp houqp requested a review from ashb April 8, 2022 19:59
@ashb ashb merged commit 9769a65 into apache:main Apr 8, 2022
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashb ashb ashb approved these changes

@potiuk potiuk potiuk approved these changes

@kaxil kaxil Awaiting requested review from kaxil kaxil is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

@uranusjr uranusjr Awaiting requested review from uranusjr

Assignees
No one assigned
Labels
area:Scheduler Scheduler or dag parsing Issues 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@houqp @potiuk @uranusjr @ashb @ephraimbuddy @dimonchik-suvorov