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

Merge GCSObjectExistenceAsyncSensor logic to GCSObjectExistenceSensor #30014

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Mar 10, 2023


Why making this change?

apache-airflow-providers-google treats the deferred execution of its operators and sensors in a different way which might cause confusion. For example, BigQueryCheckOperator uses the deferrable parameter to toggle deferred execution while we need to use another sensor for GCSObjectExistenceSensor (i.e., GCSObjectExistenceAsyncSensor) to achieve the same thing.

What's changed?

In this pull request, I move the deferred logic from GCSObjectExistenceAsyncSensor to GCSObjectExistenceSensor so that we can keep the consistency and reduce maintenance effort. Instead of removing GCSObjectExistenceAsyncSensor, I add a deprecation warning in case there're users using it.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Mar 10, 2023
@Lee-W Lee-W force-pushed the merge-async-logic-from-GCSObjectExistenceAsyncSensorto-GCSObjectExistenceSens branch 7 times, most recently from 1679cc2 to dc22aa0 Compare March 10, 2023 20:52
@Lee-W Lee-W force-pushed the merge-async-logic-from-GCSObjectExistenceAsyncSensorto-GCSObjectExistenceSens branch from dc22aa0 to fe7843c Compare March 20, 2023 00:45
Comment on lines -173 to +180
Use the :class:`~airflow.providers.google.cloud.sensors.gcs.GCSObjectExistenceAsyncSensor`
(deferrable version) if you would like to free up the worker slots while the sensor is running.
:class:`~airflow.providers.google.cloud.sensors.gcs.GCSObjectExistenceAsyncSensor` is deprecated and will be removed in a future release. Please use :class:`~airflow.providers.google.cloud.sensors.gcs.GCSObjectExistenceSensor` and use the deferrable mode in that operator.
Copy link
Member

Choose a reason for hiding this comment

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

I don’t recall our deprecation strategy around providers; should the documentation simply be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we remove the documentation here, the CI will not pass. It says we miss the example DAG for GCSObjectExistenceAsyncSensor

@uranusjr uranusjr changed the title Merge async logic from GCSObjectExistenceAsyncSensor to GCSObjectExistenceSens Merge async logic from GCSObjectExistenceAsyncSensor to GCSObjectExistenceSensor Mar 20, 2023
@uranusjr uranusjr changed the title Merge async logic from GCSObjectExistenceAsyncSensor to GCSObjectExistenceSensor Merge GCSObjectExistenceAsyncSensor logic to GCSObjectExistenceSensor Mar 20, 2023
@Lee-W Lee-W force-pushed the merge-async-logic-from-GCSObjectExistenceAsyncSensorto-GCSObjectExistenceSens branch from fe7843c to 8dc4197 Compare March 20, 2023 14:20
…syncSensor to GCSObjectExistenceSensor

add deprecation warning to GCSObjectExistenceAsyncSensor
…eferrable mode and GCSObjectExistenceAsyncSensor deprecation
@Lee-W Lee-W force-pushed the merge-async-logic-from-GCSObjectExistenceAsyncSensorto-GCSObjectExistenceSens branch from 8dc4197 to 15f4b4e Compare March 20, 2023 14:24
@Lee-W
Copy link
Member Author

Lee-W commented Mar 21, 2023

@uranusjr I noticed some of the CI steps are skipped. Should we restart them?

@uranusjr
Copy link
Member

Looks like the worker timed out. Retrying.

@potiuk
Copy link
Member

potiuk commented Mar 21, 2023

Nice!

@potiuk potiuk merged commit 79a2fa7 into apache:main Mar 21, 2023
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants