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.

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

[AIRFLOW-6250] handle_failure needs better default for context and test_mode #6812

Merged
merged 1 commit into from
Dec 16, 2019

Conversation

yuqian90
Copy link
Contributor

@yuqian90 yuqian90 commented Dec 13, 2019

Make sure you have checked all steps below.

Jira

Description

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

on_failure_callback almost always want to know the dag_id and taskinstance that failed. These info are in the context passed to on_failure_callback, which is passed in from handle_failure(). However, in some rare scenarios, if handle_failure is called in scheduler_job.py and backfill_job.py, the only argument passed is the error message. context is left as None.

So in these cases, on_failure_callback will not even know what's the dag_id of the dag that just failed.

This PR fixes this by setting context to get_template_context() if it's not given.

Tests

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

Added test in test_taskinstance.py

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

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Small style changes, otherwise looks good.

tests/models/test_taskinstance.py Outdated Show resolved Hide resolved
tests/models/test_taskinstance.py Outdated Show resolved Hide resolved
@yuqian90 yuqian90 force-pushed the fix_no_context branch 3 times, most recently from 818a566 to d56bf10 Compare December 14, 2019 03:32
@codecov-io
Copy link

Codecov Report

Merging #6812 into master will decrease coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6812      +/-   ##
==========================================
- Coverage    84.6%   84.31%   -0.29%     
==========================================
  Files         676      676              
  Lines       38331    38335       +4     
==========================================
- Hits        32431    32324     -107     
- Misses       5900     6011     +111
Impacted Files Coverage Δ
airflow/models/taskinstance.py 93.98% <100%> (+0.18%) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 78.2% <0%> (-20.52%) ⬇️
airflow/utils/dag_processing.py 87.42% <0%> (-0.58%) ⬇️
airflow/models/taskfail.py 100% <0%> (+4.34%) ⬆️

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 8107651...8e7305e. Read the comment docs.

@yuqian90 yuqian90 requested a review from ashb December 16, 2019 08:24
@ashb ashb merged commit 1006740 into apache:master Dec 16, 2019
kaxil pushed a commit that referenced this pull request Dec 18, 2019
@yuqian90 yuqian90 deleted the fix_no_context branch December 18, 2019 12:55
ashb pushed a commit that referenced this pull request Dec 19, 2019
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
…xt (apache#6812)

on_failure_callback almost always want to know the dag_id and taskinstance that failed. These info are in the context passed to on_failure_callback, which is passed in from handle_failure(). However, in some rare scenarios, if handle_failure is called in scheduler_job.py and backfill_job.py, the only argument passed is the error message. context is left as None.

So in these cases, on_failure_callback will not even know what's the dag_id of the dag that just failed.

This PR fixes this by setting context to get_template_context() if it's not given.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants