[AIRFLOW-2923][AIRFLOW-1784] Implement LatestOnlyOperator as BaseBranchOperator #5970

m1racoli · 2019-08-31T11:45:00Z

LatestOnlyOperator is a special case of a BranchOperator, thus it should inherit from it.
This fixes an issue where the skipping behaviour of LatestOnlyOperator is inconsistent with other operators,
by forcefully skipping all downstream tasks recursively ignoring trigger rules.

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-2923
- https://issues.apache.org/jira/browse/AIRFLOW-1784

Description

Here are some details about my PR, including screenshots of any UI changes:

LatestOnlyOperator is a special case of a BranchOperator, thus it should inherit from it.
This fixes an issue where the skipping behaviour of LatestOnlyOperator is inconsistent with other operators by forcefully skipping all downstream tasks recursively, ignoring trigger rules.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Extended tests.operators.test_latest_only_operator.py to cover downstream children with trigger rules. Furthermore fixed the test to not skip task in externally triggered DagRuns.

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

Passes flake8

feluelle

The implementation looks good to me but can you also update the docs accordingly? :)

Your changes on the one hand are fixing the issues but on the other hand are changing the expected behavior related to the docs

task3 is downstream of task1 and task2 and because of the default trigger_rule being all_success will receive a cascaded skip from task1. task4 is downstream of task1 and task2. It will be first skipped directly by LatestOnlyOperator, even its trigger_rule is set to all_done.

(https://airflow.apache.org/concepts.html#latest-run-only)
In both cases it should skip downstream tasks regardless of its trigger_rules.

Can you also fix its pylint issues if there are any. It is currently not being checked by pylint due to pylint_todo.txt. Do you mind removing the related lines in there (latest_only_operator and test_latest_only_operator) and fix issues if there are any? :) Thanks.

feluelle · 2019-10-25T15:14:21Z

I think it would also be nice if we have some documentation about the expected behavior of dagruns executed with external_trigger=True.

OmerJog · 2019-11-17T11:12:55Z

@m1racoli are you working on this PR?

m1racoli · 2019-11-17T19:12:41Z

Hey @OmerJog,
I just came back from vacation. Yes, I'll work on this PR. I should find some time in the next weeks.

stale · 2020-01-01T19:33:59Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

VigoTen · 2020-01-04T17:40:01Z

The LatestOnlyOperator is indeed inconsistent with how other operators handle the tasks downstream i'm really looking forward for it to be fixed

m1racoli · 2020-01-07T22:05:20Z

Documentation updated and pylint issues fixed.

codecov-io · 2020-01-07T23:11:54Z

Codecov Report

Merging #5970 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5970      +/-   ##
==========================================
+ Coverage   85.34%   85.34%   +<.01%     
==========================================
  Files         791      791              
  Lines       40128    40127       -1     
==========================================
+ Hits        34247    34248       +1     
+ Misses       5881     5879       -2

Impacted Files	Coverage Δ
airflow/operators/latest_only_operator.py	`100% <100%> (+10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c890d06...eba7aaf. Read the comment docs.

feluelle · 2020-01-22T10:03:28Z

@m1racoli do you think we should add a note to the Updating.md so that people are aware that now the trigger rules will be checked correctly/differently ?

feluelle · 2020-01-22T10:04:47Z

docs/concepts.rst

-``task2``. It will be first skipped directly by ``LatestOnlyOperator``,
-even its ``trigger_rule`` is set to ``all_done``.
+``task2``, but it will not be skipped, since its ``trigger_rule`` is set to
+``all_done``.


I think this should also be added to the Updating.md (see my comment).

That is something users should be aware of when updating Airflow. Don't you think?

Agree with @feluelle , let's add this to Updating.md

Good work @m1racoli

Good point and thanks! Will update Updating.md.

m1racoli · 2020-01-22T21:57:00Z

Comments for updating have been added.

kaxil · 2020-01-23T08:46:22Z

UPDATING.md

+
+In previous versions the `LatestOnlyOperator` forcefully skippded all (direct and undirect) downstream tasks on it's own. From this version on the operator will **only skip direct downstream** tasks and the scheduler will handle skipping any further downstream dependencies.
+
+No change is needed, if only the default trigger rule `all_success` is beeing used.


Suggested change

No change is needed, if only the default trigger rule `all_success` is beeing used.

No change is needed if only the default trigger rule `all_success` is being used.

kaxil · 2020-01-23T08:46:39Z

UPDATING.md

@@ -943,6 +943,16 @@ The following metrics are deprecated and won't be emitted in Airflow 2.0:
 - `dag.loading-duration.<basename>` -- use `dag_processing.last_duration.<basename>` instead
 - `dag_processing.last_runtime.<basename>` -- use `dag_processing.last_duration.<basename>` instead

+### Changes to skipping behaviour of LatestOnlyOperator
+
+In previous versions the `LatestOnlyOperator` forcefully skippded all (direct and undirect) downstream tasks on it's own. From this version on the operator will **only skip direct downstream** tasks and the scheduler will handle skipping any further downstream dependencies.


Suggested change

In previous versions the `LatestOnlyOperator` forcefully skippded all (direct and undirect) downstream tasks on it's own. From this version on the operator will **only skip direct downstream** tasks and the scheduler will handle skipping any further downstream dependencies.

In previous versions, the `LatestOnlyOperator` forcefully skipped all (direct and undirect) downstream tasks on its own. From this version on the operator will **only skip direct downstream** tasks and the scheduler will handle skipping any further downstream dependencies.

…chOperator LatestOnlyOperator is a special case of a BranchOperator, thus it should inherit from it. This fixes an issue where the skipping behaviour of LatestOnlyOperator is inconsistent with other operators, by forcefully skipping all downstream tasks recursively ignoring trigger rules.

kaxil · 2020-01-27T08:43:10Z

Good work @m1racoli

…chOperator (apache#5970) LatestOnlyOperator is a special case of a BranchOperator, thus it should inherit from it. This fixes an issue where the skipping behaviour of LatestOnlyOperator is inconsistent with other operators, by forcefully skipping all downstream tasks recursively ignoring trigger rules.

m1racoli force-pushed the feature/AIRFLOW-2923 branch from 4b5fe80 to 78184b8 Compare September 8, 2019 11:14

kaxil force-pushed the master branch from 558f9f1 to d719e1f Compare October 2, 2019 15:30

feluelle reviewed Oct 25, 2019

View reviewed changes

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 1, 2020

m1racoli force-pushed the feature/AIRFLOW-2923 branch from 78184b8 to 24fbf5d Compare January 7, 2020 21:07

stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 7, 2020

m1racoli force-pushed the feature/AIRFLOW-2923 branch from 24fbf5d to f224d38 Compare January 7, 2020 21:10

boring-cyborg bot added the area:dev-tools label Jan 7, 2020

m1racoli force-pushed the feature/AIRFLOW-2923 branch from f224d38 to ceaf360 Compare January 7, 2020 22:03

boring-cyborg bot added the area:docs label Jan 7, 2020

feluelle approved these changes Jan 8, 2020

View reviewed changes

feluelle reviewed Jan 22, 2020

View reviewed changes

m1racoli force-pushed the feature/AIRFLOW-2923 branch 2 times, most recently from 3dfafa2 to eba7aaf Compare January 22, 2020 21:56

kaxil reviewed Jan 23, 2020

View reviewed changes

m1racoli force-pushed the feature/AIRFLOW-2923 branch from eba7aaf to d600f6e Compare January 23, 2020 09:04

kaxil merged commit b568f74 into apache:master Jan 27, 2020

m1racoli deleted the feature/AIRFLOW-2923 branch January 30, 2020 12:35

kaxil mentioned this pull request Feb 3, 2020

[AIRFLOW-XXXX] Move LatestOnlyOperator change to Master #7350

Merged

6 tasks

boittega mentioned this pull request Apr 13, 2020

Docs: LatestOnlyOperator documentation is misleading #8246

Closed

yuqian90 mentioned this pull request Jun 27, 2020

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

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-2923][AIRFLOW-1784] Implement LatestOnlyOperator as BaseBranchOperator #5970

[AIRFLOW-2923][AIRFLOW-1784] Implement LatestOnlyOperator as BaseBranchOperator #5970

m1racoli commented Aug 31, 2019

feluelle left a comment

feluelle commented Oct 25, 2019

OmerJog commented Nov 17, 2019

m1racoli commented Nov 17, 2019

stale bot commented Jan 1, 2020

VigoTen commented Jan 4, 2020

m1racoli commented Jan 7, 2020

codecov-io commented Jan 7, 2020 •

edited

feluelle commented Jan 22, 2020

feluelle Jan 22, 2020

feluelle Jan 22, 2020

kaxil Jan 22, 2020

m1racoli Jan 22, 2020

m1racoli commented Jan 22, 2020

kaxil Jan 23, 2020

m1racoli Jan 23, 2020

kaxil Jan 23, 2020

m1racoli Jan 23, 2020

kaxil commented Jan 27, 2020


		In previous versions the `LatestOnlyOperator` forcefully skippded all (direct and undirect) downstream tasks on it's own. From this version on the operator will only skip direct downstream tasks and the scheduler will handle skipping any further downstream dependencies.

		No change is needed, if only the default trigger rule `all_success` is beeing used.

	No change is needed, if only the default trigger rule `all_success` is beeing used.
	No change is needed if only the default trigger rule `all_success` is being used.

	In previous versions the `LatestOnlyOperator` forcefully skippded all (direct and undirect) downstream tasks on it's own. From this version on the operator will only skip direct downstream tasks and the scheduler will handle skipping any further downstream dependencies.
	In previous versions, the `LatestOnlyOperator` forcefully skipped all (direct and undirect) downstream tasks on its own. From this version on the operator will only skip direct downstream tasks and the scheduler will handle skipping any further downstream dependencies.

[AIRFLOW-2923][AIRFLOW-1784] Implement LatestOnlyOperator as BaseBranchOperator #5970

[AIRFLOW-2923][AIRFLOW-1784] Implement LatestOnlyOperator as BaseBranchOperator #5970

Conversation

m1racoli commented Aug 31, 2019

Jira

Description

Tests

Commits

Documentation

Code Quality

feluelle left a comment

Choose a reason for hiding this comment

feluelle commented Oct 25, 2019

OmerJog commented Nov 17, 2019

m1racoli commented Nov 17, 2019

stale bot commented Jan 1, 2020

VigoTen commented Jan 4, 2020

m1racoli commented Jan 7, 2020

codecov-io commented Jan 7, 2020 • edited

Codecov Report

feluelle commented Jan 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1racoli commented Jan 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaxil commented Jan 27, 2020

codecov-io commented Jan 7, 2020 •

edited