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.

Sign up for GitHub

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

Jump to bottom

Refactor BaseSensorOperator introduce skip_policy parameter #40924

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Refactor BaseSensorOperator introduce skip_policy parameter #40924

merged 2 commits into from
Jul 25, 2024

Conversation

raphaelauv
Copy link
Contributor

@raphaelauv raphaelauv commented Jul 22, 2024 •
edited
Loading

fix: #40915

  • remove never_fail parameter
  • introduce skip_policy parameter
  • deprecate soft_fail and silent_fail parameters

@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Jul 22, 2024
@raphaelauv raphaelauv force-pushed the fix_sensor_skip_policy branch 7 times, most recently from d460b23 to 7308b52 Compare July 22, 2024 09:39
@raphaelauv
Copy link
Contributor Author

@Lee-W @shahar1 @eladkal wdyt ? thanks

shahar1
shahar1 reviewed Jul 22, 2024
View reviewed changes
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

Looks good overall - had some comments, most of them are regarding documenation and terminology.
@Lee-W - I'd be happy if you could also review it.

airflow/sensors/base.py Outdated Show resolved Hide resolved
airflow/sensors/base.py Outdated Show resolved Hide resolved
airflow/sensors/base.py Outdated Show resolved Hide resolved
airflow/sensors/base.py Outdated Show resolved Hide resolved
eladkal
eladkal approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Nice!

@eladkal eladkal changed the title fix: sensor - skip_policy Refactor BaseSensorOperator introduce skip_policy parameter Jul 22, 2024
@raphaelauv raphaelauv mentioned this pull request Jul 22, 2024
Closed
@Lee-W Lee-W self-requested a review July 22, 2024 23:53
Lee-W
Lee-W reviewed Jul 23, 2024
View reviewed changes
airflow/sensors/base.py Show resolved Hide resolved
airflow/sensors/base.py Show resolved Hide resolved
airflow/sensors/base.py Show resolved Hide resolved
airflow/sensors/base.py Show resolved Hide resolved
@raphaelauv
Copy link
Contributor Author

@eladkal should we merge this part and continue to iterate with the next PR's or wait for another review of @Lee-W ? thanks

@raphaelauv
Copy link
Contributor Author

@eladkal gentle ping ( I would like to finish this work for airflow 2.10 , so we can trust sensors again in airflow , cause right now we can't have at least once pattern with the soft_fail option )

@eladkal eladkal added this to the Airflow 2.10.0 milestone Jul 25, 2024
@eladkal eladkal added the type:improvement Changelog: Improvements label Jul 25, 2024
@eladkal eladkal merged commit ba78d54 into apache:main Jul 25, 2024
48 checks passed
@raphaelauv raphaelauv deleted the fix_sensor_skip_policy branch July 25, 2024 16:38
@potiuk
Copy link
Member

potiuk commented Jul 26, 2024

Hey @raphaelauv -> This one broke our canary builds https://github.com/apache/airflow/actions/runs/10107479883/job/27951619130 -> can you also apply the new policy to all the operators that are raising warnings now ?

@potiuk
Copy link
Member

potiuk commented Jul 26, 2024

Warnings detected in test case(s):

  • tests/providers/apache/flink/sensors/test_flink_kubernetes.py::TestFlinkKubernetesSensor::test_cluster_error_state[True-AirflowSkipException]
  • tests/providers/cncf/kubernetes/sensors/test_spark_kubernetes.py::TestSparkKubernetesSensor::test_driver_logging_failure[True-AirflowSkipException]
  • tests/providers/cncf/kubernetes/sensors/test_spark_kubernetes.py::TestSparkKubernetesSensor::test_failed_application[True-AirflowSkipException]
  • tests/providers/cncf/kubernetes/sensors/test_spark_kubernetes.py::TestSparkKubernetesSensor::test_unknown_application[True-AirflowSkipException]
  • tests/providers/datadog/sensors/test_datadog.py::TestDatadogSensor::test_sensor_fail_with_exception[True-AirflowSkipException]
  • tests/providers/http/sensors/test_http.py::TestHttpSensor::test_poke_exception_with_soft_fail

@raphaelauv
Copy link
Contributor Author

@potiuk yes I started the replace in this PR -> #40923

i'm going to finish in few hours

@potiuk
Copy link
Member

potiuk commented Jul 26, 2024

BTW, This should be made compatible with Airflow 2.7 + (maybe we could use common.compat provider.

In tne meantime, I will revert this one and maybe it will be better if you combine the two together this time ?

potiuk added a commit to potiuk/airflow that referenced this pull request Jul 26, 2024
@potiuk
Revert "Refactor BaseSensorOperator introduce skip_policy paramet…
5033ad4 
…er (apache#40924)"

This reverts commit ba78d54.

This is needed because provider tests started to fail and this one
should be combined and tested wiht apache#40923 including compatibility.
@potiuk potiuk mentioned this pull request Jul 26, 2024
Merged
potiuk added a commit that referenced this pull request Jul 26, 2024
@potiuk
Revert "Refactor BaseSensorOperator introduce skip_policy paramet…
e0ccaa9 
…er (#40924)" (#41045)

This reverts commit ba78d54.

This is needed because provider tests started to fail and this one
should be combined and tested wiht #40923 including compatibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lee-W Lee-W Lee-W left review comments

@eladkal eladkal eladkal approved these changes

@shahar1 shahar1 shahar1 approved these changes

Assignees
No one assigned
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow type:improvement Changelog: Improvements
Projects
None yet
Milestone
Airflow 2.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@raphaelauv @potiuk @Lee-W @eladkal @shahar1