You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "michaelpri10 (via GitHub)" <gi...@apache.org> on 2026/05/27 21:48:56 UTC
[PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
michaelpri10 opened a new pull request, #67621:
URL: https://github.com/apache/airflow/pull/67621
Make `return_immediately` default to False and be configurable for all Pub/Sub modules. This field is [deprecated](https://docs.cloud.google.com/pubsub/docs/reference/rest/v1/projects.subscriptions/pull?rep_location=global#request-body) and [Cloud Pub/Sub documentation warns](https://docs.cloud.google.com/pubsub/docs/pull-troubleshooting#using_the_option_returnimmediately) that this can lead to pull requests returning with 0 messages even when there is a backlog. By default, the `return_immediately` option should be set to false.
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: http://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ X] Yes (please specify the tool below)
Generated-by: Google Antigravity following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-5653753070
@michaelpri10 I've been AFK for quite a while, now I'm back.
Would you be able to finalize this PR? (fixing failing tests + addressing Makims's comments)
Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3447348740
##########
providers/google/src/airflow/providers/google/cloud/sensors/pubsub.py:
##########
@@ -113,7 +113,7 @@ def __init__(
project_id: str,
subscription: str,
max_messages: int = 5,
- return_immediately: bool = True,
+ return_immediately: bool = False,
Review Comment:
Same comment applies to the rest of the changed operators:
As long as this field is still usable, I'd rather deprecate it slowly from Airflow to prevent sudden changes for Airflow's users. According to the Google provider [deprecation policy](https://airflow.apache.org/docs/apache-airflow-providers-google/stable/deprecation-policy.html#additional-considerations), we usually allow 6 months until making breaking changes - but since this is already deprecated in the API and might already cause unwanted behavior, I'm fine with breaking it in the version after the deprecation is released (about a month from now).
Let's change the default back to `True`, but with a deprecation warning that it will be changed to `False` in the next breaking version due to the circumstances.
When it's published that the feature is finally removed from the API, we could also remove the field entirely (after a similar deprecation cycle).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4943210536
@michaelpri10 please review current CI failures
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3641329772
##########
providers/google/src/airflow/providers/google/cloud/operators/pubsub.py:
##########
@@ -831,6 +841,17 @@ def __init__(
self.impersonation_chain = impersonation_chain
self.deferrable = deferrable
self.poll_interval = poll_interval
+ if return_immediately is not None:
+ warnings.warn(
+ "The `return_immediately` parameter is deprecated and will be removed in a future release. "
+ "Its default value will be changed to `False` in the next major release. "
+ "Planned removal date: August 01, 2026.",
Review Comment:
I've updated the deprecation warning to just state that the default value of `return_immediately` will be set to `False` in a future major release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "MaksYermak (via GitHub)" <gi...@apache.org>.
MaksYermak commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3711787722
##########
providers/google/src/airflow/providers/google/cloud/operators/pubsub.py:
##########
Review Comment:
@michaelpri10 did you run the system tests for these changes with `return_immediately=False`?
I am not sure that we should make this parameter configurable for `PubSubPullOperator` as I understand the idea of this operator was return any value immediately include 0 if user does not have any messages. I think it was a reason why we have hardcoded `return_immediately=True` here. Otherwise with `return_immediately=False` it blocks whole task until message will not appear in my opinion this behavior contradicts with operator logic. If users need blocking waiting for this reason we have `PubSubPullSensor`.
It is the message from docstring to this operator:
```
Pulls messages from a PubSub subscription and passes them through XCom.
If the queue is empty, returns empty list - never waits for messages.
If you do need to wait, please use :class:`airflow.providers.google.cloud.sensors.PubSubPullSensor`
instead.
```
##########
providers/google/src/airflow/providers/google/cloud/sensors/pubsub.py:
##########
@@ -127,13 +129,21 @@ def __init__(
self.project_id = project_id
self.subscription = subscription
self.max_messages = max_messages
- self.return_immediately = return_immediately
self.ack_messages = ack_messages
self.messages_callback = messages_callback
self.impersonation_chain = impersonation_chain
self.deferrable = deferrable
self.poke_interval = poke_interval
self._return_value = None
+ if return_immediately is not None:
+ warnings.warn(
+ "The default value of `return_immediately` will be changed to `False` in a future major release.",
+ AirflowProviderDeprecationWarning,
+ stacklevel=2,
+ )
Review Comment:
@michaelpri10 in google provider we have deprecation policy and by this policy you need to specify planned_removal_date and by default all deprecations should allow a 6 months time period until they will be removed and not available. Could you please add the removal date to this warning message?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3447348740
##########
providers/google/src/airflow/providers/google/cloud/sensors/pubsub.py:
##########
@@ -113,7 +113,7 @@ def __init__(
project_id: str,
subscription: str,
max_messages: int = 5,
- return_immediately: bool = True,
+ return_immediately: bool = False,
Review Comment:
Same comment applies to the rest of the changed operators:
As long as this field is still usable, I'd rather deprecate it slowly from Airflow to prevent sudden changes for Airflow's users. According to the Google provider [deprecation policy](https://airflow.apache.org/docs/apache-airflow-providers-google/stable/deprecation-policy.html#additional-considerations), we usually allow 6 months until making breaking changes - but since this is already deprecated in the API and might already cause unwanted behavior, I'm fine with breaking it in the version after the deprecation is released (about a month from now).
Let's change the default back to `True`, but with a deprecation warning that it will be changed to `False` in the next breaking version.
When it's published that the feature is finally removed from the API, we could also remove the field entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3447348740
##########
providers/google/src/airflow/providers/google/cloud/sensors/pubsub.py:
##########
@@ -113,7 +113,7 @@ def __init__(
project_id: str,
subscription: str,
max_messages: int = 5,
- return_immediately: bool = True,
+ return_immediately: bool = False,
Review Comment:
Same comment applies to the rest of the changed operators:
As long as this field is still usable, I'd rather deprecate it slowly from Airflow to prevent sudden changes for Airflow's users. According to the Google provider [deprecation policy](https://airflow.apache.org/docs/apache-airflow-providers-google/stable/deprecation-policy.html#additional-considerations), we usually allow 6 months until making breaking changes - but since this is already deprecated in the API and might cause unwanted behavior, I'm fine with breaking it in the version after the deprecation is released (about a month from now).
Let's change the default back to `True`, but with a deprecation warning that it will be changed to `False` in the next breaking version.
When it's published that the feature is finally removed from the API, we could also remove the field entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 closed pull request #67621: Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated
URL: https://github.com/apache/airflow/pull/67621
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3560272917
##########
providers/google/src/airflow/providers/google/cloud/sensors/pubsub.py:
##########
@@ -113,7 +113,7 @@ def __init__(
project_id: str,
subscription: str,
max_messages: int = 5,
- return_immediately: bool = True,
+ return_immediately: bool = False,
Review Comment:
I've made this change to default to `True` and left a comment with some additional info at https://github.com/apache/airflow/pull/67621#issuecomment-4895915816.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3640245297
##########
providers/google/src/airflow/providers/google/cloud/hooks/pubsub.py:
##########
@@ -712,6 +721,15 @@ async def pull(
the base64-encoded message content. See
https://cloud.google.com/pubsub/docs/reference/rpc/google.pubsub.v1#google.pubsub.v1.ReceivedMessage
"""
+ if return_immediately is not None:
+ warnings.warn(
+ "The `return_immediately` parameter is deprecated and will be removed in a future release.",
+ AirflowProviderDeprecationWarning,
+ stacklevel=2,
+ )
Review Comment:
When the parameter is forwarded down from an operator, it will be passed a boolean (after it had been transformed there) - which will always trigger this warning, regardles if being set or not in the operator's level.
##########
providers/google/src/airflow/providers/google/cloud/operators/pubsub.py:
##########
@@ -831,6 +841,17 @@ def __init__(
self.impersonation_chain = impersonation_chain
self.deferrable = deferrable
self.poll_interval = poll_interval
+ if return_immediately is not None:
+ warnings.warn(
+ "The `return_immediately` parameter is deprecated and will be removed in a future release. "
+ "Its default value will be changed to `False` in the next major release. "
+ "Planned removal date: August 01, 2026.",
Review Comment:
1. Please don't mix up two deprecations in one warning, this is very confusing for the end-users. Let's focus the current deprecation only on changing the behavior, and only when Google publishes an official date for removing the parameter - then we could deprecate it towards removal.
2. Speaking of an official date - I didn't find any mention in Google API for Aug. 01 as the removal date, so I wouldn't mention it here at all as well.
##########
providers/google/tests/unit/google/cloud/hooks/test_pubsub.py:
##########
@@ -28,10 +28,13 @@
from google.cloud.pubsub_v1.types import PublisherOptions, ReceivedMessage
from googleapiclient.errors import HttpError
+from airflow.exceptions import AirflowProviderDeprecationWarning
from airflow.providers.google.cloud.hooks.pubsub import PubSubAsyncHook, PubSubException, PubSubHook
from airflow.providers.google.common.consts import CLIENT_INFO
from airflow.version import version
+pytestmark = pytest.mark.filterwarnings("ignore::airflow.exceptions.AirflowProviderDeprecationWarning")
+
Review Comment:
This warning is directly related to the comment on the hook :)
You wouldn't need it if the hook won't raise the warning for every case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4798268610
@michaelpri10 — this PR has 1 unresolved review thread(s) that still look like they need your attention. Once you've addressed them (push changes and/or reply in-thread), please resolve the threads or reply to confirm, and give the reviewer a nudge for another look. Thanks!
See the [PR quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria).
<sub>Automated first-pass triage note drafted by an AI-assisted tool — may get things wrong; once addressed, a real Apache Airflow maintainer takes the next look. ([why automated](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md))</sub>
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4693755217
> @michaelpri10 A few things need addressing before review — see our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria).
>
> * ❌ **Provider tests**. See [docs](https://github.com/apache/airflow/blob/main/contributing-docs/12_provider_distributions.rst).
>
> No rush.
>
> _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._
This change does contain updated provider unit tests to test the relevant changes, which I believe should satisfy the testing criteria.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3572773381
##########
providers/google/tests/unit/google/cloud/operators/test_pubsub.py:
##########
@@ -526,9 +542,12 @@ def test_execute_deferred(self, mock_hook):
subscription=TEST_SUBSCRIPTION,
deferrable=True,
)
- with pytest.raises(TaskDeferred) as _:
+ with pytest.raises(TaskDeferred) as exc:
task.execute(mock.MagicMock())
+ assert isinstance(exc.value.trigger, PubsubPullTrigger)
+ assert exc.value.trigger.return_immediately is True
Review Comment:
I've added a test to cover this.
##########
providers/google/tests/unit/google/cloud/sensors/test_pubsub.py:
##########
Review Comment:
I've added a test to cover this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4960769803
> @michaelpri10 please review current CI failures
@shahar1 I've updated the tests to fix the current CI failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4895915816
I've updated the PR to change back to setting `return_immediately==True` by default with a deprecation notice per [this comment](https://github.com/apache/airflow/pull/67621/changes#r3447348740). I will note that `PubSubHook` currently [defaults to `return_immediately==False`](https://github.com/apache/airflow/blob/ff10b2e3e19c9a6c1a262ebd32ef65b968b39424/providers/google/src/airflow/providers/google/cloud/hooks/pubsub.py#L495), which has been in place since at least https://github.com/apache/airflow/pull/6096 (~7 years ago), so I've left that as is.
With regards to the initial attempt to remove the `return_immediately` field in https://github.com/apache/airflow/pull/23231 and the subsequent issue in https://github.com/apache/airflow/issues/41838, the cause of that issue was because the field defaulted to `True`. This specific issue is noted in the [Cloud Pub/Sub documentation](https://docs.cloud.google.com/pubsub/docs/pull-troubleshooting#using_the_option_returnimmediately):
> If your client is using unary Pull, check if the returnImmediately field is set to true. This is a deprecated field that tells the Pub/Sub service to respond to the pull request immediately even if it returns with no messages. This can result in pull requests returning with 0 messages even when there is a backlog.
Thus, I think it would be beneficial to eventually switch the default to `False` (and remove the field completely later on).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4658174028
Hello @shahar1! Gentle ping on the review for this PR, thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-5061116754
@shahar1 Had to make a few more changes to fix some tests, but all checks are passing now!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "nailo2c (via GitHub)" <gi...@apache.org>.
nailo2c commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4964634338
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4612886842
The failing test looks to be unrelated to the changes here and possibly caused by a misconfiguration of GitHub actions (or maybe a transient infrastructure error) due to the error:
```
Error: Can't find 'action.yml', 'action.yaml' or 'Dockerfile' under '/home/runner/work/airflow/airflow/.github/actions/post_tests_failure'. Did you forget to run actions/checkout before running your local action?
```
<img width="2559" height="1226" alt="Screenshot 2026-06-03 at 9 33 19 AM" src="https://github.com/user-attachments/assets/264bbabd-8754-45bb-9533-a9f8e04bb872" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3641337430
##########
providers/google/tests/unit/google/cloud/hooks/test_pubsub.py:
##########
@@ -28,10 +28,13 @@
from google.cloud.pubsub_v1.types import PublisherOptions, ReceivedMessage
from googleapiclient.errors import HttpError
+from airflow.exceptions import AirflowProviderDeprecationWarning
from airflow.providers.google.cloud.hooks.pubsub import PubSubAsyncHook, PubSubException, PubSubHook
from airflow.providers.google.common.consts import CLIENT_INFO
from airflow.version import version
+pytestmark = pytest.mark.filterwarnings("ignore::airflow.exceptions.AirflowProviderDeprecationWarning")
+
Review Comment:
As noted [here](https://github.com/apache/airflow/pull/67621#discussion_r3641335222), this is no longer needed, so I've removed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-5063350501
> Almost there!
Thank you @shahar1! I've addressed the comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-5753879299
This pull request has been automatically marked as stale because the author has not responded to a request for more information. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-5775370541
Replaced by #73504
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4558938564
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our [Contributors' Guide](https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:
- Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example Dag that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices).
- Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "MaksYermak (via GitHub)" <gi...@apache.org>.
MaksYermak commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4769131708
> Thanks for your contribution and apologies for the delay in response!
>
> CC: @VladaZakharova @MaksYermak - I'll appreciate your opinion here, thanks!
@shahar1 thank you for letting us know!
I agree with you that for this code the default value should change back to `True`.
**About deprecation.**
We already had had deprecation warning for this code in the past and `return_immediately` parameter was removed by this https://github.com/apache/airflow/pull/23231. After that because of [issue](https://github.com/apache/airflow/issues/41838) in `PubSubPullSensor ` it was retuned back by this https://github.com/apache/airflow/pull/41842. The current PR extends the usage for this deprecated parameter in my opinion it does not make sense because it is deprecated and can be removed any time. But I think we need to make investigate what we can use instead of this parameter for being sure that after removing the `PubSubPullSensor` will work as expect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #67621:
URL: https://github.com/apache/airflow/pull/67621#issuecomment-4657827958
@michaelpri10 A few things need addressing before review — see our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria).
- :x: **Provider tests**. See [docs](https://github.com/apache/airflow/blob/main/contributing-docs/12_provider_distributions.rst).
No rush.
_Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately default to False and be configurable for all Pub/Sub modules [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3447348740
##########
providers/google/src/airflow/providers/google/cloud/sensors/pubsub.py:
##########
@@ -113,7 +113,7 @@ def __init__(
project_id: str,
subscription: str,
max_messages: int = 5,
- return_immediately: bool = True,
+ return_immediately: bool = False,
Review Comment:
Same comment applies to the rest of the changes operators:
As long as this field is still usable, I'd rather deprecate it slowly from Airflow to prevent sudden changes for Airflow's users. According to the Google provider [deprecation policy](https://airflow.apache.org/docs/apache-airflow-providers-google/stable/deprecation-policy.html#additional-considerations), we usually allow 6 months until making breaking changes - but since this is already deprecated in the API and might cause unwanted behavior, I'm fine with breaking it in the version after the deprecation is released (about a month from now).
Let's change the default back to `True`, but with a deprecation warning that it will be changed to `False` in the next breaking version.
When it's published that the feature is finally removed from the API, we could also remove the field entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "nailo2c (via GitHub)" <gi...@apache.org>.
nailo2c commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3567696957
##########
providers/google/tests/unit/google/cloud/sensors/test_pubsub.py:
##########
Review Comment:
Maybe we also need to test `return_immediately=False` here?
##########
providers/google/tests/unit/google/cloud/operators/test_pubsub.py:
##########
@@ -526,9 +542,12 @@ def test_execute_deferred(self, mock_hook):
subscription=TEST_SUBSCRIPTION,
deferrable=True,
)
- with pytest.raises(TaskDeferred) as _:
+ with pytest.raises(TaskDeferred) as exc:
task.execute(mock.MagicMock())
+ assert isinstance(exc.value.trigger, PubsubPullTrigger)
+ assert exc.value.trigger.return_immediately is True
Review Comment:
I saw other test functions cover `return_immediately=False`, so I think `test_execute_deferred` should cover it as well to prevent a regression in the deferrable path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]
Posted by "michaelpri10 (via GitHub)" <gi...@apache.org>.
michaelpri10 commented on code in PR #67621:
URL: https://github.com/apache/airflow/pull/67621#discussion_r3641335222
##########
providers/google/src/airflow/providers/google/cloud/hooks/pubsub.py:
##########
@@ -712,6 +721,15 @@ async def pull(
the base64-encoded message content. See
https://cloud.google.com/pubsub/docs/reference/rpc/google.pubsub.v1#google.pubsub.v1.ReceivedMessage
"""
+ if return_immediately is not None:
+ warnings.warn(
+ "The `return_immediately` parameter is deprecated and will be removed in a future release.",
+ AirflowProviderDeprecationWarning,
+ stacklevel=2,
+ )
Review Comment:
Given that the `PubSubHook` class already has `False` as the default value, the updated deprecation warning no longer applies here, so I've undone the changes to this class (and its test class).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org