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-5653] Log caught AirflowSkipException in task instance log #6330

Merged
merged 2 commits into from
Oct 14, 2019

Conversation

rajatsri28
Copy link
Contributor

@rajatsri28 rajatsri28 commented Oct 14, 2019

Jira

Description

  • Logging any caught AirflowSkipException with message in models/taskinstance.py
    to remove confusion due to no task skipping information being present in the logs

Tests

  • My PR does not needs any new tests since its just adding a single log line

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

  • No new functionality introduced

Logging any caught AirflowSkipException with message in models/taskinstance.py
to remove confusion due to no task skipping information in the logs
except AirflowSkipException as e:
# log only if exception has any arguments to prevent log flooding
if e.args:
self.log.warning(e)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be at info level? It doesn't really seem like a "warning" to me.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Since task skipping is not so much of a warning in airflow. I have made the change. Thanks for the tip.

@codecov-io
Copy link

codecov-io commented Oct 14, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6330      +/-   ##
==========================================
+ Coverage   80.34%   80.34%   +<.01%     
==========================================
  Files         616      616              
  Lines       35737    35739       +2     
==========================================
+ Hits        28713    28715       +2     
  Misses       7024     7024
Impacted Files Coverage Δ
airflow/models/taskinstance.py 93.28% <100%> (+0.02%) ⬆️

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 0a99aad...30ff89b. Read the comment docs.

@aoen
Copy link
Contributor

aoen commented Oct 14, 2019

Have you tested this on a local instance?

@rajatsri28
Copy link
Contributor Author

Yes. They ran fine on my local instance. In fact, the travis-ci test was fine in my first commit too and the only change in the second commit is the log level.
Seems like the build failed due to some unrelated reasons?

@aoen
Copy link
Contributor

aoen commented Oct 14, 2019

Failing test looks flaky and unrelated, merging.

Here is the failing test for context:

:::test_dag-test_task:[2019-10-14 14:53:07,279] {base_task_runner.py:125} INFO - Running: ['airflow', 'tasks', 'run', 'test_heartbeat_failed_fast', 'test_heartbeat_failed_fast_op', '2016-01-01T00:00:00+00:00', '--raw', '-sd', 'DAGS_FOLDER/test_heartbeat_failed_fast.py', '--cfg_path', '/tmp/tmp579tpcvb']

::::test_dag-test_task:[2019-10-14 14:53:08,975] {base_task_runner.py:111} INFO - Job None: Subtask test_heartbeat_failed_fast_op /opt/airflow/airflow/models/dagbag.py:21: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses

::::test_dag-test_task:[2019-10-14 14:53:08,978] {base_task_runner.py:111} INFO - Job None: Subtask test_heartbeat_failed_fast_op import imp

::::test_dag-test_task:[2019-10-14 14:53:09,074] {base_task_runner.py:111} INFO - Job None: Subtask test_heartbeat_failed_fast_op [2019-10-14 14:53:09,073] {settings.py:175} INFO - settings.configure_orm(): Using pool settings. pool_size=5, max_overflow=10, pool_recycle=1800, pid=8013

::::test_dag-test_task:[2019-10-14 14:53:09,492] {base_task_runner.py:111} INFO - Job None: Subtask test_heartbeat_failed_fast_op [2019-10-14 14:53:09,491] {init.py:53} INFO - Using executor LocalExecutor

::::test_dag-test_task:[2019-10-14 14:53:09,493] {base_task_runner.py:111} INFO - Job None: Subtask test_heartbeat_failed_fast_op [2019-10-14 14:53:09,492] {dagbag.py:90} INFO - Filling up the DagBag from /opt/airflow/tests/dags/test_heartbeat_failed_fast.py

::::test_dag-test_task:[2019-10-14 14:53:09,518] {base_task_runner.py:111} INFO - Job None: Subtask test_heartbeat_failed_fast_op [2019-10-14 14:53:09,518] {cli.py:590} INFO - Running <TaskInstance: test_heartbeat_failed_fast.test_heartbeat_failed_fast_op 2016-01-01T00:00:00+00:00 [running]> on host 4a17e03fc9e0

FAILED

@aoen aoen changed the title AIRFLOW-5653 Log caught AirflowSkipException in task instance log [AIRFLOW-5653] Log caught AirflowSkipException in task instance log Oct 14, 2019
@aoen aoen merged commit 1a4c164 into apache:master Oct 14, 2019
@rajatsri28 rajatsri28 deleted the AIRFLOW-5653 branch October 14, 2019 18:42
ashb pushed a commit that referenced this pull request Oct 16, 2019
…6330)

[AIRFLOW-5653] Log caught AirflowSkipException in task instance log

Logging any caught AirflowSkipException with message in models/taskinstance.py
to remove confusion due to no task skipping information in the logs

(cherry picked from commit 1a4c164)
kaxil pushed a commit to astronomer/airflow that referenced this pull request Jul 16, 2020
…pache#6330)

[AIRFLOW-5653] Log caught AirflowSkipException in task instance log

Logging any caught AirflowSkipException with message in models/taskinstance.py
to remove confusion due to no task skipping information in the logs

(cherry picked from commit 1a4c164)
(cherry picked from commit a04cafd)
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.

4 participants