[AIRFLOW-6535] add exception to fail without retry #7133

jstern · 2020-01-10T18:15:43Z

I would like to be able to configure certain tasks to retry, but to also have a way to bypass retry if I can detect a condition that is unlikely to be change for the better.

For example, imagine I have a DAG with a large number of tasks that fetch data from an API ... sometimes the API returns a 200 and everything is fine, sometimes the API returns a 500 and I know this means that API is failing under load and I want to retry later, and sometimes the API returns a 400 indicating that I've configured my requests in a way that will never succeed. If the API returns a 400 for some/all of my tasks, then I have to wait for all of them to get through all their retries before the run fails.

My proposal is to add another subclass of AirflowException called AirflowFailException, and to update the exception handling in TaskInstance.run_raw_task such that when this exception is seen, the resulting behavior is that same as if we entered handle_failure and is_eligible_for_returned false (except with adjusted logging so it does not look like we simply hit our retry limit).

Issue link: AIRFLOW-6535

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
Unit tests coverage for changes (not needed for documentation changes)
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.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

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.

airflow/models/taskinstance.py

potiuk

I like it, just one small comment. It looks like a good way. However it will also need some description in the documentation and some tests.

jstern · 2020-01-10T18:32:10Z

I like it, just one small comment. It looks like a good way. However it will also need some description in the documentation and some tests.

Thanks for the quick review! Definitely will follow up with some tests shortly.

I was looking for a good place in the docs ... ideally I would have found an existing spot that discusses the semantics of AirflowSkipException and added something there, but the docs don't cover that either :/ Is there a particular spot within the documentation that would be best?

jstern · 2020-01-10T18:36:49Z

One other question: am I perhaps overcomplicating this? It occurs to me that if a task encounters an AirflowSkipException, we don't bother checking on its retry status or adjusting our messaging based on that:

        except AirflowSkipException as e:
            # Recording SKIP
            # log only if exception has any arguments to prevent log flooding
            if e.args:
                self.log.info(e)
            self.refresh_from_db(lock_for_update=True)
            self.state = State.SKIPPED
            self.log.info(
                'Marking task as SKIPPED.'
                'dag_id=%s, task_id=%s, execution_date=%s, start_date=%s, end_date=%s',
                self.dag_id,
                self.task_id,
                self.execution_date.strftime('%Y%m%dT%H%M%S') if hasattr(
                    self,
                    'execution_date') and self.execution_date else '',
                self.start_date.strftime('%Y%m%dT%H%M%S') if hasattr(
                    self,
                    'start_date') and self.start_date else '',
                self.end_date.strftime('%Y%m%dT%H%M%S') if hasattr(
                    self,
                    'end_date') and self.end_date else '')

Maybe instead of messing with handle_failure I should just emulate that but with different messaging? Might be cleaner and easier to follow...

EDIT: but then I would still need to make sure failure email/callbacks happen ... so I guess I can either leave this structured the way I have it or include those in the except block.

potiuk · 2020-01-10T18:39:59Z

I like it, just one small comment. It looks like a good way. However it will also need some description in the documentation and some tests.

Thanks for the quick review! Definitely will follow up with some tests shortly.

I was looking for a good place in the docs ... ideally I would have found an existing spot that discusses the semantics of AirflowSkipException and added something there, but the docs don't cover that either :/ Is there a particular spot within the documentation that would be best?

I think "Concepts" is best and adding chapter about Exceptions would be good.

potiuk · 2020-01-10T18:40:33Z

Maybe instead of messing with handle_failure I should just emulate that but with different messaging? Might be cleaner and easier to follow...

It's ok. I think it's easy enough.

codecov-io · 2020-01-10T22:50:06Z

Codecov Report

Merging #7133 into master will decrease coverage by 54.47%.
The diff coverage is 7.5%.

@@             Coverage Diff             @@
##           master    #7133       +/-   ##
===========================================
- Coverage   86.92%   32.45%   -54.48%     
===========================================
  Files         915      914        -1     
  Lines       44152    44152               
===========================================
- Hits        38381    14329    -24052     
- Misses       5771    29823    +24052

Impacted Files	Coverage Δ
airflow/models/dag.py	`46.41% <0%> (-44.81%)`	⬇️
airflow/exceptions.py	`96.66% <100%> (-3.34%)`	⬇️
airflow/models/taskinstance.py	`29.1% <5.55%> (-65.88%)`	⬇️
...low/contrib/operators/wasb_delete_blob_operator.py	`0% <0%> (-100%)`	⬇️
airflow/contrib/hooks/vertica_hook.py	`0% <0%> (-100%)`	⬇️
airflow/contrib/sensors/__init__.py	`0% <0%> (-100%)`	⬇️
airflow/hooks/mssql_hook.py	`0% <0%> (-100%)`	⬇️
...viders/docker/example_dags/example_docker_swarm.py	`0% <0%> (-100%)`	⬇️
airflow/hooks/webhdfs_hook.py	`0% <0%> (-100%)`	⬇️
airflow/contrib/sensors/emr_base_sensor.py	`0% <0%> (-100%)`	⬇️
... and 786 more

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 35eff0a...a887d1f. Read the comment docs.

robinedwards · 2020-01-24T16:36:48Z

Interesting I have a hack in one of my operators which allows the selection of specific exceptions to retry for.

jstern · 2020-01-24T17:45:03Z

Interesting I have a hack in one of my operators which allows the selection of specific exceptions to retry for.

I'd be curious to see how you do it ... maybe it's not really that hacky? :) I'd be happy to close this PR if there's already a sensible/easy way to accomplish this within what airflow already provides...

airflow/models/taskinstance.py

robinedwards · 2020-01-27T09:52:11Z

@jstern https://gist.github.com/robinedwards/3f2ec4336e1ced084547d24d7e7ead3a

airflow/models/taskinstance.py

ashb

Nice idea.

The one thing I don't think the tests cover is that actually raising this new exception causes the task to not be retried -- you've tested the change to handle_failure, but that is essentially the internal details, rather than the "interface" of the new feature.

airflow/models/taskinstance.py

docs/conf.py

tests/models/test_taskinstance.py

jstern · 2020-02-04T15:13:59Z

The one thing I don't think the tests cover is that actually raising this new exception causes the task to not be retried -- you've tested the change to handle_failure, but that is essentially the internal details, rather than the "interface" of the new feature.

I'll take some time to look deeper but I didn't find any existing tests of the interface for handling the other exceptions, so I glommed onto the closest test I could find. If you know offhand where better example tests that I can extend please point me to them :) Or I can try to take some time to add them.

KevinYang21 · 2020-03-27T21:37:30Z

Hi @jstern , this is a very nice feature that we are also very interested in. Do we still plan to merge it? Anything we can do to help?

jstern · 2020-03-27T22:23:29Z

Thanks @KevinYang21 - I definitely would like to see it get merged, but I'm stuck on a strange problem with the docs build throwing warnings due to the problem I described in #7133 (comment) above.

The short version of the issue is that my PR adds airflow.exceptions to the API docs, but this causes warnings in the doc build because AirflowException is already included (incorrectly) in the yandex provider API docs. I have not yet had the time or inspiration to figure out why that bug exists in master or how to correct it.

Another option that would let the build succeed would be to revert the change that adds the exceptions module to the api docs, but since I believe the docs in master are wrong I have been reluctant to take this step.

Perhaps I should file a JIRA ticket for the docs bug in master and hope someone who understands Sphinx and autoapi better than I do knows how to fix it?

KevinYang21 · 2020-04-09T20:37:58Z

@jstern sry I was distracted, yes I think your plan is good. Saw that you got a PR fixing doc, will look into that one too. Thank you!

tooptoop4 · 2020-04-16T20:32:12Z

gentle ping @jstern

jstern · 2020-04-17T00:31:09Z

gentle ping @jstern

hi @tooptoop4 ! just waiting on #8095 to hit master so the docs can build cleanly and then hopefully we'll be able to get this one ready to merge

Co-Authored-By: Jarek Potiuk <[email protected]>

Try to reduce nesting and repetition of logic for different conditions. Also try to tighten up the scope of the exception handling ... it looks like the large block that catches an Exception and logs it as a failure to send an email may have been swallowing some TypeErrors coming out of trying to compose a log info message and calling strftime on start_date and end_date when they're set to None; this is why I've added lines in the test to set those values on the TaskInstance objects.

tooptoop4 · 2020-05-02T16:43:12Z

bump?

jstern · 2020-05-16T14:33:42Z

@KevinYang21 if you have some time to look at this again ... nothing I'm aware of to hold it up other than committer approval.

cc @ashb @potiuk ... i know you guys are very busy but i'd be very grateful if you could find time to take another look at this :)

ashb

one tiny doc change then I'm happy with this.

sorry you had to chase so many times!

docs/concepts.rst

…to airflow-fail-exception

KevinYang21 · 2020-05-25T06:24:23Z

🎉 I need to really spend some time on my email filter, keep missing such important PR @ :) Thank you @jstern

marshall7m · 2020-06-03T03:03:26Z

Hi there, just wondering what version of airflow the AirflowFailException is available on. Thanks!

ashb · 2020-06-03T09:28:25Z

It's marked to be included in 1.10.11

) * use preferred boolean check idiom Co-Authored-By: Jarek Potiuk <[email protected]> * add test coverage for AirflowFailException * add docs for some exception usage patterns * autoformatting * remove extraneous newline, poke travis build * clean up TaskInstance.handle_failure Try to reduce nesting and repetition of logic for different conditions. Also try to tighten up the scope of the exception handling ... it looks like the large block that catches an Exception and logs it as a failure to send an email may have been swallowing some TypeErrors coming out of trying to compose a log info message and calling strftime on start_date and end_date when they're set to None; this is why I've added lines in the test to set those values on the TaskInstance objects. * let sphinx generate docs for exceptions module * keep session kwarg last in handle_failure * explain allowed_top_level * add black-box tests for retry/fail immediately cases * don't lose safety measures in logging date attrs * fix flake8 too few blank lines * grammar nitpick * add import to AirflowFailException example Co-authored-by: Jarek Potiuk <[email protected]> (cherry picked from commit 707bb0c)

* use preferred boolean check idiom Co-Authored-By: Jarek Potiuk <[email protected]> * add test coverage for AirflowFailException * add docs for some exception usage patterns * autoformatting * remove extraneous newline, poke travis build * clean up TaskInstance.handle_failure Try to reduce nesting and repetition of logic for different conditions. Also try to tighten up the scope of the exception handling ... it looks like the large block that catches an Exception and logs it as a failure to send an email may have been swallowing some TypeErrors coming out of trying to compose a log info message and calling strftime on start_date and end_date when they're set to None; this is why I've added lines in the test to set those values on the TaskInstance objects. * let sphinx generate docs for exceptions module * keep session kwarg last in handle_failure * explain allowed_top_level * add black-box tests for retry/fail immediately cases * don't lose safety measures in logging date attrs * fix flake8 too few blank lines * grammar nitpick * add import to AirflowFailException example Co-authored-by: Jarek Potiuk <[email protected]> (cherry picked from commit 707bb0c)

potiuk reviewed Jan 10, 2020

View reviewed changes

airflow/models/taskinstance.py Outdated Show resolved Hide resolved

potiuk reviewed Jan 10, 2020

View reviewed changes

jstern changed the title ~~[AIRFLOW-6535] first pass at exception to fail without retry~~ [AIRFLOW-6535] add exception to fail without retry Jan 10, 2020

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

jstern requested a review from potiuk January 24, 2020 14:37

houqp reviewed Jan 24, 2020

View reviewed changes

airflow/models/taskinstance.py Outdated Show resolved Hide resolved

ashb reviewed Feb 4, 2020

View reviewed changes

airflow/models/taskinstance.py Show resolved Hide resolved

ashb reviewed Feb 4, 2020

View reviewed changes

airflow/models/taskinstance.py Outdated Show resolved Hide resolved

docs/conf.py Show resolved Hide resolved

tests/models/test_taskinstance.py Outdated Show resolved Hide resolved

jstern mentioned this pull request Apr 3, 2020

stop rendering some class docs in wrong place #8095

Merged

5 tasks

jstern force-pushed the airflow-fail-exception branch 2 times, most recently from f661cbd to f19e950 Compare April 26, 2020 12:53

jstern and others added 5 commits April 26, 2020 13:43

first pass at exception to fail without retry

f32ac45

add test coverage for AirflowFailException

b4d0af9

use preferred boolean check idiom

88aec7d

Co-Authored-By: Jarek Potiuk <[email protected]>

add docs for some exception usage patterns

4ccc5bb

autoformatting

2472113

jstern added 9 commits April 26, 2020 13:43

remove extraneous newline, poke travis build

f0939b9

let sphinx generate docs for exceptions module

2e5028d

keep session kwarg last in handle_failure

b9d57c6

explain allowed_top_level

3e4d25d

add black-box tests for retry/fail immediately cases

9a7b500

don't lose safety measures in logging date attrs

53dc373

fix flake8 too few blank lines

cf122b7

grammar nitpick

6bf74ad

jstern force-pushed the airflow-fail-exception branch from f19e950 to 6bf74ad Compare April 26, 2020 18:43

tooptoop4 approved these changes May 2, 2020

View reviewed changes

Merge branch 'master' into airflow-fail-exception

1769728

ashb requested changes May 16, 2020

View reviewed changes

docs/concepts.rst Show resolved Hide resolved

jstern added 2 commits May 16, 2020 11:20

add import to AirflowFailException example

6bddec2

Merge branch 'airflow-fail-exception' of github.com:jstern/airflow in…

48bea5e

…to airflow-fail-exception

ashb approved these changes May 16, 2020

View reviewed changes

ashb merged commit 707bb0c into apache:master May 16, 2020

ashb added this to the Airflow 1.10.11 milestone May 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-6535] add exception to fail without retry #7133

[AIRFLOW-6535] add exception to fail without retry #7133

jstern commented Jan 10, 2020 •

edited by boring-cyborg bot

Loading

potiuk left a comment

jstern commented Jan 10, 2020

jstern commented Jan 10, 2020 •

edited

Loading

potiuk commented Jan 10, 2020

potiuk commented Jan 10, 2020

codecov-io commented Jan 10, 2020 •

edited

Loading

robinedwards commented Jan 24, 2020

jstern commented Jan 24, 2020

robinedwards commented Jan 27, 2020

ashb left a comment

jstern commented Feb 4, 2020

KevinYang21 commented Mar 27, 2020

jstern commented Mar 27, 2020

KevinYang21 commented Apr 9, 2020

tooptoop4 commented Apr 16, 2020

jstern commented Apr 17, 2020

tooptoop4 commented May 2, 2020

jstern commented May 16, 2020

ashb left a comment

KevinYang21 commented May 25, 2020

marshall7m commented Jun 3, 2020

ashb commented Jun 3, 2020

[AIRFLOW-6535] add exception to fail without retry #7133

[AIRFLOW-6535] add exception to fail without retry #7133

Conversation

jstern commented Jan 10, 2020 • edited by boring-cyborg bot Loading

potiuk left a comment

Choose a reason for hiding this comment

jstern commented Jan 10, 2020

jstern commented Jan 10, 2020 • edited Loading

potiuk commented Jan 10, 2020

potiuk commented Jan 10, 2020

codecov-io commented Jan 10, 2020 • edited Loading

Codecov Report

robinedwards commented Jan 24, 2020

jstern commented Jan 24, 2020

robinedwards commented Jan 27, 2020

ashb left a comment

Choose a reason for hiding this comment

jstern commented Feb 4, 2020

KevinYang21 commented Mar 27, 2020

jstern commented Mar 27, 2020

KevinYang21 commented Apr 9, 2020

tooptoop4 commented Apr 16, 2020

jstern commented Apr 17, 2020

tooptoop4 commented May 2, 2020

jstern commented May 16, 2020

ashb left a comment

Choose a reason for hiding this comment

KevinYang21 commented May 25, 2020

marshall7m commented Jun 3, 2020

ashb commented Jun 3, 2020

jstern commented Jan 10, 2020 •

edited by boring-cyborg bot

Loading

jstern commented Jan 10, 2020 •

edited

Loading

codecov-io commented Jan 10, 2020 •

edited

Loading