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

Fix cancelling queries for Redshift/Postgres #661

Merged
merged 4 commits into from Nov 24, 2015
Merged

Fix cancelling queries for Redshift/Postgres #661

merged 4 commits into from Nov 24, 2015

Conversation

alexdebrie
Copy link
Contributor

Fixes #598.

I tested this on our stage machine and it worked. I've only actually implemented a handler for Postgres, but any query_runner can catch the InterruptException and handle it as desired.

I'm open to any and all comments. We don't have any other datasources hooked up, so not sure how this affects the other query runners.

@arikfr
Copy link
Member

arikfr commented Nov 22, 2015

When I was testing this I noticed that it crashed my server, because it was trying to install the handler on non main thread. I think it has to do with PyCharm's debugger, but it made me realize that installing the handler every time we create the a query runner object is not necessary (and maybe even wrong).

I moved the signal.signal call to the execute_query function of redash.tasks and it works but prevents the issue I experienced (ideally we will call it once when the Celery worker starts, but didn't have the chance to look into it, and it felt good enough).

As for impact on other query runners: I've tested it only with MySQL, and it looks like it doesn't change the behavior, so looks like it's safe on that front. But queries don't get really cancelled... we will need to address it at some point, but once you move the signal.signal call it's good for merging.

@alexdebrie
Copy link
Contributor Author

Thanks for debugging and tips. I moved the signal handler as requested and tested on our machine. As a note, I left the InterruptException within query_runners as this is something other runners can import and catch as they see fit.

Let me know if you need any other changes.

arikfr added a commit that referenced this pull request Nov 24, 2015
Fix cancelling queries for Redshift/Postgres
@arikfr arikfr merged commit 0343fa7 into getredash:master Nov 24, 2015
@arikfr
Copy link
Member

arikfr commented Nov 24, 2015

Thanks! Merged.

dairyo pushed a commit to KiiCorp/redash that referenced this pull request Mar 1, 2019
Fix cancelling queries for Redshift/Postgres
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants