-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Fix RedshiftDataOperator not running in deferred mode when it should #41206
Conversation
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)
|
Wasn't this already fixed in #41191 ? |
At first I thought too, but it's a different |
Ah OK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Elad, a unit test would be great, but otherwise LGTM
Here's what I did:
Not sure if it's a good idea to iterate over Warning I realize now that this fix will change the operator's behavior: for folks who have been using it with |
You are correct but this is a bug fix. The previous behavior was wrong so to me we should go ahead with that change |
Can you add to the provider changelog a block of
to the changelog where you explain how to mitigate the change. It needs to have 2-4 sentenses that just users would understand what was change and how to mitigate this. Example: https://github.com/apache/airflow/blob/main/airflow/providers/amazon/CHANGELOG.rst#870 |
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
…pache#41206) * Ensure operator goes into deferrable mode * Remove commented out code * Test when not waiting for completion * Add entry to changelog * Rephrase warning
Problems
RedshiftDataOperator
task is configured withdeferrable=True
andwait_for_completion=True
(the default), it doesn't go indeferred
state. Instead it stays inrunning
state until the statement completes.wait_for_completion=False
anddeferrable=True
, after the statement is submitted, the task will still go intodeferred
mode and wait for the statement to complete.Reasons
deferrable=True
,self.wait_for_completion
is set toFalse
inexecute()
, but never used after.execute
does not check whether the task should wait for completion, only if it should be deferred.Solution
wait_for_completion
instead ofself.wait_for_completion
when deferrable. Also, remove redundant condition onself.wait_for_completion
How I tested
Checked with this simple DAG that the operator now behaves as expected, for all 4 combinations:
^ 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.