Skip to content
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 logic to handle on_kill for BigQueryInsertJobOperator when deferrable=True #38912

Merged
merged 5 commits into from
Apr 15, 2024

Conversation

sunank200
Copy link
Collaborator

@sunank200 sunank200 commented Apr 10, 2024

This pull request aims to improve the cancellation logic of the BigQueryInsertJobOperator when set to deferrable=True. Previously, cancel_on_kill=True worked well in non-deferrable mode but did not function as expected in deferrable=True mode. This bug has been resolved with this change. However, the bug has a larger scope and is a generic issue in the trigger. An issue has already been raised on Airflow regarding this matter and can be found at #36090.


^ 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.

@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Apr 10, 2024
@sunank200 sunank200 force-pushed the fix-bigquery-insert-job-on-kill branch 3 times, most recently from 3fe5af1 to a5cd68d Compare April 12, 2024 06:30
@sunank200 sunank200 force-pushed the fix-bigquery-insert-job-on-kill branch from a5cd68d to 4078368 Compare April 12, 2024 13:26
@sunank200 sunank200 marked this pull request as ready for review April 12, 2024 15:12
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it looks great! Left a few nitpicks

@sunank200 sunank200 force-pushed the fix-bigquery-insert-job-on-kill branch from 289520c to 1b234c4 Compare April 15, 2024 06:37
@sunank200 sunank200 requested a review from Lee-W April 15, 2024 06:52
@sunank200 sunank200 self-assigned this Apr 15, 2024
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left one nitpick

airflow/providers/google/cloud/hooks/bigquery.py Outdated Show resolved Hide resolved
@sunank200 sunank200 force-pushed the fix-bigquery-insert-job-on-kill branch from 6503101 to a3dbbf2 Compare April 15, 2024 08:36
@Lee-W
Copy link
Member

Lee-W commented Apr 15, 2024

This PR looks good to me. I'm planning on merging it later today. But please let me know if anyone wants to take a deeper look 🙂

@sunank200 sunank200 force-pushed the fix-bigquery-insert-job-on-kill branch from a3dbbf2 to a2311c7 Compare April 15, 2024 08:58
@Lee-W Lee-W merged commit e237041 into apache:main Apr 15, 2024
40 of 41 checks passed
@Lee-W Lee-W deleted the fix-bigquery-insert-job-on-kill branch April 15, 2024 11:06
@tirkarthi
Copy link
Contributor

@sunank200 As I understand this will kill the job when triggerer is restarted for deployment, maintenance etc or marked unhealthy in HA mode with the trigger to be handled by another triggerer job as the task is cancelled by asyncio and the CancellationError is caught in except clause to kill the job. Is it expected that triggerer restarts should kill the job?

@thesuperzapper
Copy link
Contributor

@Lee-W @sunank200 @tirkarthi this PR is NOT safe, and needs to be reverted or updated.

Please see my #36090 (comment) for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants