-
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
Add support for cancelling job when deferrable Databricks operator is killed #39373
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)
|
Won't there need to be an update to the operators too, since they currently just call the sync I think either that or the hook.cancel_run can accept a |
I don't believe so since the trigger executes in a completely different context than the operators. The changes here are specifically to account for the following scenario:
Currently, terminating the task via the UI doesn't propagate that information back to Databricks that the job should be terminated in this situation. From my testing locally, the changes in this PR should remedy the situation. Open to other implementation alternatives, but this is a pretty similar approach as other providers as I noted in the PR description. I did initially attempt to implement a Trigger.cleanup() method, but that specific function can also be executed within the context of a triggerer restart which isn't ideal behaviour since the Databricks job should continue running across triggerer restarts, especially for long-running Databricks jobs. |
@akaul @RNHTTR this PR is NOT safe, and needs to be updated. Please see my #36090 (comment) for more information. |
@thesuperzapper Thanks for the information! This feels like it should be tackled at the Airflow core level as a high priority bug rather than each provider implementing their own solution for this. The workaround you've proposed makes sense, but it feels like too much overhead for each provider to implement in their triggers when the |
I agree, but I believe the fix in core Airflow is not straightforward. @sunank200 can you provide some additional info with regard to the level of difficulty in core Airflow? |
PR's been updated to inspect for the task instance's state before cancelling the Databricks job. |
This pull request updates the behaviour of deferrable Databricks operators to handle manual terminations more gracefully when a Databricks operator running in deferrable mode is terminated (generally by a user manually restarting or failing a task).
To handle this scenario, we catch
asyncio.CancelledError
that's thrown, check if the job is in a non-terminal state and then proceed with cancelling it if it is. This takes a similar approach as other providers ([1], [2]) to gracefully terminate these jobs as the standardon_kill
method on the operator doesn't work.Unlike other operators, an explicit
cancel_on_kill
input hasn't been added here, but happy to include a new parameter for the trigger based on feedback.See #36090 for the general issue around this.
[1] #38912
[2] https://github.com/apache/airflow/blob/main/airflow/providers/google/cloud/triggers/dataproc.py#L125-L138
^ 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.