Re: [PR] Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated [airflow]

["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