Skip to content

[AIRFLOW-5882] Add ti_dep for not being in the RUNNING state#6531

Merged
potiuk merged 1 commit intoapache:masterfrom
saguziel:aguziel-2019-11-airflow-ignore-state-dep-apache
Nov 12, 2019
Merged

[AIRFLOW-5882] Add ti_dep for not being in the RUNNING state#6531
potiuk merged 1 commit intoapache:masterfrom
saguziel:aguziel-2019-11-airflow-ignore-state-dep-apache

Conversation

@saguziel
Copy link
Contributor

@saguziel saguziel commented Nov 8, 2019

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-5882
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    This ti_state dep should not be ignoreable as it would allow double-run in the "ideal" situation and in the current situation it causes both tasks to die. Due to celery visibility timeout, the task will always kill itself eventually

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    will fix existing unit tests if they exist

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

@saguziel saguziel force-pushed the aguziel-2019-11-airflow-ignore-state-dep-apache branch from 7a420e9 to 3f78324 Compare November 8, 2019 22:08
@saguziel saguziel changed the title [AIRFLOW-5882] ti_state_dep should not be ignoreable [AIRFLOW-5882] Add ti_dep for not being in the RUNNING state Nov 8, 2019
@saguziel saguziel force-pushed the aguziel-2019-11-airflow-ignore-state-dep-apache branch 4 times, most recently from 3176168 to 5f70467 Compare November 9, 2019 02:06
@OmerJog
Copy link
Contributor

OmerJog commented Nov 10, 2019

@saguziel
There are style errors:

airflow/ti_deps/deps/task_not_running_dep.py:1:0: C0111: Missing module docstring (missing-docstring)
airflow/ti_deps/deps/task_not_running_dep.py:25:0: C0111: Missing class docstring (missing-docstring)
airflow/ti_deps/deps/task_not_running_dep.py:36:15: C0123: Using type() instead of isinstance() for a typecheck. (unidiomatic-typecheck)
airflow/ti_deps/deps/task_not_running_dep.py:42:4: W0222: Signature differs from overridden '_get_dep_statuses' method (signature-differs)
************* Module airflow.utils.log.json_formatter
airflow/utils/log/json_formatter.py:1:0: R0401: Cyclic import (airflow -> airflow.executors -> airflow.executors.kubernetes_executor -> airflow.kubernetes.pod_launcher) (cyclic-import)
airflow/utils/log/json_formatter.py:1:0: R0401: Cyclic import (airflow.executors -> airflow.executors.kubernetes_executor -> airflow.kubernetes.pod_generator) (cyclic-import)
airflow/utils/log/json_formatter.py:1:0: R0401: Cyclic import (airflow.executors -> airflow.executors.kubernetes_executor -> airflow.kubernetes.pod_launcher -> airflow.kubernetes.pod_generator) (cyclic-import)

@saguziel saguziel force-pushed the aguziel-2019-11-airflow-ignore-state-dep-apache branch 2 times, most recently from 5019f57 to 507419b Compare November 12, 2019 01:32
@saguziel saguziel force-pushed the aguziel-2019-11-airflow-ignore-state-dep-apache branch from 507419b to 9bf9f42 Compare November 12, 2019 01:44
@codecov-io
Copy link

codecov-io commented Nov 12, 2019

Codecov Report

Merging #6531 into master will decrease coverage by 0.28%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6531      +/-   ##
==========================================
- Coverage   84.07%   83.78%   -0.29%     
==========================================
  Files         639      641       +2     
  Lines       36900    36919      +19     
==========================================
- Hits        31024    30934      -90     
- Misses       5876     5985     +109
Impacted Files Coverage Δ
airflow/ti_deps/deps/__init__.py 100% <100%> (ø)
airflow/ti_deps/dep_context.py 100% <100%> (ø) ⬆️
airflow/ti_deps/deps/task_not_running_dep.py 94.11% <94.11%> (ø)
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/kube_client.py 33.33% <0%> (-41.67%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 70.14% <0%> (-28.36%) ⬇️
airflow/utils/dag_processing.py 58.15% <0%> (-0.33%) ⬇️
... and 3 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 d25ef2b...9bf9f42. Read the comment docs.

@potiuk potiuk merged commit aeb4a26 into apache:master Nov 12, 2019
GnunuX pushed a commit to GnunuX/airflow that referenced this pull request Nov 13, 2019
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.

6 participants