Skip to content

[AIRFLOW-5391] Do not re-run skipped tasks when they are cleared (#7276) #8992

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
Merged
kaxil merged 4 commits into from
Jul 22, 2020
Merged

[AIRFLOW-5391] Do not re-run skipped tasks when they are cleared (#7276) #8992

kaxil merged 4 commits into from
Jul 22, 2020

Conversation

@yuqian90
Copy link
Contributor

@yuqian90 yuqian90 commented May 24, 2020

This PR is backported from #7276. The original commit was merged into master but not released in v1-10-*. This PR fixes a few minor merge conflicts and python2.7 compatibility issues and port it to v1-10-test.

If a task is skipped by BranchPythonOperator, BaseBranchOperator or ShortCircuitOperator and the user then clears the skipped task later, it'll execute. This is probably not the right
behaviour.

This commit changes that so it will be skipped again. This can be ignored by running the task again with "Ignore Task Deps" override.

(cherry picked from commit 1cdab56)

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

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.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label May 24, 2020
@yuqian90 yuqian90 closed this May 24, 2020
@yuqian90 yuqian90 reopened this May 24, 2020
@yuqian90 yuqian90 force-pushed the v1-10-test_only_AIRFLOW-5391 branch from 5acf5a5 to 91dc27d Compare May 26, 2020 13:56
@dimberman dimberman force-pushed the v1-10-test branch 5 times, most recently from 7595db8 to 3a4e73c Compare May 28, 2020 18:31
@yuqian90 yuqian90 force-pushed the v1-10-test_only_AIRFLOW-5391 branch from 91dc27d to cc7ff93 Compare May 29, 2020 07:36
@dimberman dimberman force-pushed the v1-10-test branch 12 times, most recently from c4be6d2 to 0aff02b Compare May 29, 2020 23:33
@yuqian90 yuqian90 force-pushed the v1-10-test_only_AIRFLOW-5391 branch from cc7ff93 to fd3a12e Compare May 30, 2020 09:06
@dimberman dimberman force-pushed the v1-10-test branch 2 times, most recently from e676e59 to aaaa366 Compare June 2, 2020 00:58
@yuqian90 yuqian90 force-pushed the v1-10-test_only_AIRFLOW-5391 branch from fd3a12e to 84b2b41 Compare June 2, 2020 15:17
@kaxil kaxil added this to the Airflow 1.10.11 milestone Jun 2, 2020
@yuqian90 yuqian90 force-pushed the v1-10-test_only_AIRFLOW-5391 branch from 84b2b41 to 9bf3fb9 Compare June 8, 2020 09:02
@kaxil kaxil force-pushed the v1-10-test branch 7 times, most recently from eead3bc to a5a588e Compare July 2, 2020 12:25
@kaxil kaxil force-pushed the v1-10-test_only_AIRFLOW-5391 branch from a1ba607 to 654b5f1 Compare July 2, 2020 12:29
@kaxil kaxil modified the milestones: Airflow 1.10.11, Airflow 1.10.12 Jul 2, 2020
@kaxil
Copy link
Member

kaxil commented Jul 2, 2020

Hi @yuqian90, apologies this won't make it to 1.10.11 as the LatestOnlyOperator causes a change in behaviour. Is it possible for you to achieve this without changing the behaviour (talking about the note in Updating.md) or adding a flag to have old behavior vs new behaviour (default would be old behaviour).

@yuqian90 yuqian90 force-pushed the v1-10-test_only_AIRFLOW-5391 branch from 654b5f1 to 58a2d69 Compare July 11, 2020 14:31
yuqian90
yuqian90 commented Jul 11, 2020
View reviewed changes
tests/operators/test_latest_only_operator.py Outdated
Copy link
Contributor Author

@yuqian90 yuqian90 Jul 11, 2020

Choose a reason for hiding this comment

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

This fix is needed for the test because the original test calls LatestOnlyOperator.execute() without first creating a dagrun. Also the create_dag_run is moved to setUpClass so that it's called only once for the whole class.

@yuqian90
Copy link
Contributor Author

yuqian90 commented Jul 11, 2020 •
edited
Loading

Hi @yuqian90, apologies this won't make it to 1.10.11 as the LatestOnlyOperator causes a change in behaviour. Is it possible for you to achieve this without changing the behaviour (talking about the note in Updating.md) or adding a flag to have old behavior vs new behaviour (default would be old behaviour).

Hi, @kaxil sorry for the late response. I have backed out #5970 which as you have pointed out slightly changes the behaviour or LatestOnlyOperator.

I put the test fix inside the test instead (call create_dagrun() before calling LatestOnlyOperator.execute().

Looks like I missed 1.10.11. Hopefully this can make it for 1.10.12. Thanks!

@potiuk potiuk force-pushed the v1-10-test branch 2 times, most recently from 235ab10 to f1fba28 Compare July 16, 2020 14:48
@yuqian90 yuqian90 force-pushed the v1-10-test_only_AIRFLOW-5391 branch from 58a2d69 to 46df966 Compare July 21, 2020 09:24
kaxil
kaxil reviewed Jul 21, 2020
View reviewed changes
Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

Can you add a note in Updating.md and this is still a minor change in behavior for users who were clearing the skipped tasks intentionally to run it.

yuqian90 and others added 4 commits July 22, 2020 17:37
@yuqian90
[AIRFLOW-5391] Do not re-run skipped tasks when they are cleared (apa…
7e0968f 
…che#7276)

If a task is skipped by BranchPythonOperator, BaseBranchOperator or ShortCircuitOperator and the user then clears the skipped task later, it'll execute. This is probably not the right
behaviour.

This commit changes that so it will be skipped again. This can be ignored by running the task again with "Ignore Task Deps" override.

(cherry picked from commit 1cdab56)
@kaxil @yuqian90
Apply suggestions from code review
961f894
@yuqian90
Fix test_latest_only_operator by creating dag_run before setUp
87614cf
@yuqian90
Add note about SkipMixin behaviour and NotPreviouslySkippedDep
9caade2
@yuqian90 yuqian90 force-pushed the v1-10-test_only_AIRFLOW-5391 branch from 46df966 to 9caade2 Compare July 22, 2020 10:02
@yuqian90
Copy link
Contributor Author

yuqian90 commented Jul 22, 2020

Can you add a note in Updating.md and this is still a minor change in behavior for users who were clearing the skipped tasks intentionally to run it.

Hi @kaxil thanks for the suggestion. I've added a note in UPDATING.md.

@kaxil kaxil merged this pull request into apache:v1-10-test Jul 22, 2020
@kaxil
Copy link
Member

kaxil commented Jul 22, 2020

Thanks @yuqian90

@eladkal eladkal mentioned this pull request Jan 25, 2021
Closed
@yuqian90 yuqian90 deleted the v1-10-test_only_AIRFLOW-5391 branch May 29, 2021 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaxil kaxil kaxil left review comments

Assignees

No one assigned

Labels

area:Scheduler including HA (high availability) scheduler

Projects

None yet

Milestone

Airflow 1.10.12

Development

Successfully merging this pull request may close these issues.

2 participants

@yuqian90 @kaxil