-
Notifications
You must be signed in to change notification settings - Fork 529
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
Cancel link now actually cancels queries in BigQuery/Athena #1847
Conversation
Before, the "cancel" queries link would stop waiting for results and update the GrowthBook UI, but the queries would still be executing in the database in the background. Now, for BigQuery and Athena, we will send a cancel request to the database as well. Unfortunately, not all db engines support easily cancelling an in-progress query, which is why we're starting with only two.
Your preview environment pr-1847-bttf has been deployed. Preview environment endpoints are available at: |
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.
Looks good to me. Tested on BQ and it:
- Canceled my dedupe units table query that was expensive
- Updated the mongo correctly with that error
- BQ job correctly shows it was canceled by the user in the BQ UI
- Unfortunately, it left the dependent queries in "queued" status (see my comments)
So at first glance it seems sufficient and to work, but might be good to clean up a little bit after itself.
); | ||
} | ||
} | ||
|
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.
Can we also set queued and running queries to "failed"
? Otherwise we have to wait for the cleanup job that I think only handles 20 queries at a time right now.
return async () => { | ||
if (!id || !this.integration.cancelQuery) return; | ||
try { | ||
await this.integration.cancelQuery(id); |
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.
It feels like we should also run onQueryFinish()
in here, which might clean up the queued queries automatically anyways (see my below comment)
Features and Changes
Before, the "cancel" queries link would stop waiting for results and update the GrowthBook UI, but the queries would still be executing in the database in the background.
Now, for BigQuery and Athena, we will send a cancel request to the database as well.
Unfortunately, not all db engines support easily cancelling an in-progress query, which is why we're starting with only two.