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

ExternalTaskSensor should skip if soft_fail=True and external task in one of the failed_states #19754

Closed
2 tasks done
alexbegg opened this issue Nov 22, 2021 · 1 comment · Fixed by #23647
Closed
2 tasks done
Labels
area:core kind:bug This is a clearly a bug

Comments

@alexbegg
Copy link
Contributor

alexbegg commented Nov 22, 2021

Apache Airflow version

2.1.4

Operating System

Debian GNU/Linux 10 (buster)

Versions of Apache Airflow Providers

No response

Deployment

Astronomer

Deployment details

No response

What happened

I ran into a scenario where if I use an ExternalTaskSensor and set it to soft_fail=True but also set it to failed_states=['skipped'] I would expect if the external task skipped then to mark this sensor as skipped, however for the failed_states check in the poke method if it is in one of those states it will explicitly fail with an AirflowException.

Wouldn't it make more sense to skip because of the soft_fail?

What you expected to happen

The ExternalTaskSensor task should skip

How to reproduce

  1. Add a DAG with a task that is set to skip, such as this BashOperator task set to skip taken from https://airflow.apache.org/docs/apache-airflow/stable/howto/operator/bash.html#skipping:
this_will_skip = BashOperator(
    task_id='this_will_skip',
    bash_command='echo "hello world"; exit 99;',
    dag=dag,
)
  1. Add a second DAG with an ExternalTaskSensor
  2. Set that sensor to have external_dag_id be the other DAG and external_task_id be the skipped task in that other DAG and failed_states=['skipped'] and soft_fail=True
  3. The ExternalTaskSensor fails instead of skips

Anything else

I don't know what is desirable for most Airflow users:

  1. To have soft_fail to only cause skips if the sensor times out? (like it seems to currently do)
  2. To have ExternalTaskSensor with soft_fail to skip any time it would otherwise fail, such as the external task being in one of the failed_states?
  3. To have some other way for the ExternalTaskSensor to skip if the external task skipped?

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@alexbegg alexbegg added area:core kind:bug This is a clearly a bug labels Nov 22, 2021
@alexbegg
Copy link
Contributor Author

Actually, I guess I'll add to this that if ExternalTaskSensor has soft_fail=True it should maybe skip if the external task is in any of the listed failed_states?

@alexbegg alexbegg changed the title ExternalTaskSensor should skip if soft_fail=True and failed_states=skipped ExternalTaskSensor should skip if soft_fail=True and external task in one of the failed_states Nov 22, 2021
potiuk pushed a commit that referenced this issue Jun 24, 2022
…ailed_state (#23647)

* Respecting soft_fail in ExternalTaskSensor when the upstream tasks are in the failed state (#19754)

- Changed behaviour of sensor to as above to respect soft_fail
- Added tests of new soft_fail behaviour (#19754)
- Added newsfragment and improved sensor docstring
ephraimbuddy pushed a commit that referenced this issue Jul 5, 2022
…ailed_state (#23647)

* Respecting soft_fail in ExternalTaskSensor when the upstream tasks are in the failed state (#19754)

- Changed behaviour of sensor to as above to respect soft_fail
- Added tests of new soft_fail behaviour (#19754)
- Added newsfragment and improved sensor docstring

(cherry picked from commit 1b34598)
ephraimbuddy pushed a commit that referenced this issue Jul 5, 2022
…ailed_state (#23647)

* Respecting soft_fail in ExternalTaskSensor when the upstream tasks are in the failed state (#19754)

- Changed behaviour of sensor to as above to respect soft_fail
- Added tests of new soft_fail behaviour (#19754)
- Added newsfragment and improved sensor docstring

(cherry picked from commit 1b34598)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core kind:bug This is a clearly a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant