-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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 on_kill
to Databricks Workflow Operator
#42115
Add on_kill
to Databricks Workflow Operator
#42115
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)
|
70f8c96
to
c3f1e9b
Compare
c3f1e9b
to
b80e162
Compare
8a86b3b
to
ac988bb
Compare
ac988bb
to
532feb3
Compare
hey @pankajkoti - Any ETA on when you'd be able to review this PR? 🙏 |
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.
hi @R7L208 Apologies, I missed reviewing it so far.
I am happy to review the changes for databricks_workflow.py
but not so comfortable yet reviewing the changes in databricks_sql.py
(operators & hooks) as I do not have enough expertise on the changes made there. Would you please to separate out the two changes(move out databricks_sql.py related changes to another PR) and then we can invite some expert to review the other PR?
532feb3
to
10ceb61
Compare
Opened #42668 @pankajkoti @eladkal - can you please assign someone to review? |
on_kill
or equivalent to Databricks Operators/Hooks to cancel timed out querieson_kill
to Databricks Workflow Operator to cancel timed out queries
d45c9d9
to
bf5237a
Compare
@R7L208 Can you please update the PR description based on the altered scope of this PR? |
on_kill
to Databricks Workflow Operator to cancel timed out querieson_kill
to Databricks Workflow Operator
@pankajkoti - it's been updated |
bf5237a
to
bf0555b
Compare
@R7L208 it's not. It mentions about DatabricksSqlHook queries ("SQL queries submitted by DatabricksSqlHook") & using threading ("uses threading to cancel SQL queries submitted by DatabricksSqlHook.run()") which we no longer have in this PR. Could you please re-read the description and update to what's limited to the scope of this PR. Also, _CreateDatabricksWorkflowOperator does not rely on DatabricksSqlHook, but leverages DatabricksHook just in case you missed checking that :) |
@pankajkoti - Apologies! I read PR title instead of PR description 🤦 PR description is now updated |
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
* add on_kill override to databricks workflow operator * on_kill equivalent for DatabricksSqlOperator * add tests for create_timeout_thread * add note for on_kill in DatabricksCopyIntoOperator * chore: static checks * remove changes for databricks_sql.py for PR isolated to databricks_workflows.py --------- Co-authored-by: Lorin <[email protected]>
* add on_kill override to databricks workflow operator * on_kill equivalent for DatabricksSqlOperator * add tests for create_timeout_thread * add note for on_kill in DatabricksCopyIntoOperator * chore: static checks * remove changes for databricks_sql.py for PR isolated to databricks_workflows.py --------- Co-authored-by: Lorin <[email protected]>
The Databricks Provider did not implement
on_kill
to cancel tasks generated by_CreateDatabricksWorkflowOperator
. This led to data quality issues, where Airflow would report a cancellation due to timeout; however, the corresponding workflow task would continue to run on Databricks.This PR implements
on_kill
for_CreateDatabricksWorkflowOperator
.^ 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.