Skip to content

[sqllab] Cancel query to database on query stop #53

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

Merged
merged 1 commit into from
May 30, 2019
Merged

[sqllab] Cancel query to database on query stop #53

merged 1 commit into from
May 30, 2019

Conversation

smacker
Copy link
Contributor

@smacker smacker commented May 17, 2019

  • Add new attribute into Query model that keeps ID of an underlying
    connection to database for this query
  • Add get_connection_id method to EngineSpec that retrieves connection
    id for a cursor.
  • execute_sql_statements before executing queries retrieves and saves
    connection id into Query
  • Stop handler calls new cancel_query method of EngineSpec

Both new methods are implemented only for MySQL for now.
cancel_query uses KILL CONNECTION to terminate the statement.

Ref: #35

@smacker smacker requested a review from a team May 17, 2019 15:53
@smacker smacker changed the title [WIP] [sqllab] Cancel query to database on query stop [sqllab] Cancel query to database on query stop May 29, 2019
@smacker
Copy link
Contributor Author

smacker commented May 29, 2019

Tested with gitbase rc3 and it works like a charm.
It's not clear to me where and how to write tests for it, looks like superset test coverage is rather poor.
I'll propose this improvement to the upsteam and also ask for advice about tests.
But taking into account how important this feature is for our use cases I think this PR is good to be merged after code review.

Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I commented a couple of places where it seems that the only change is formatting, we may want to revert them to make it cleaner to upgrade/merge things from upstream.

@@ -193,7 +194,8 @@ def execute_sql_statement(sql_statement, query, user_name, session, cursor):
db_engine_spec.handle_cursor(cursor, query, session)

with stats_timing('sqllab.query.time_fetching_results', stats_logger):
logging.debug('Fetching data for query object: {}'.format(query.to_dict()))
logging.debug(
'Fetching data for query object: {}'.format(query.to_dict()))
Copy link
Contributor

Choose a reason for hiding this comment

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

here

@@ -308,9 +313,28 @@ def execute_sql_statements(
cache_timeout = database.cache_timeout
if cache_timeout is None:
cache_timeout = config.get('CACHE_DEFAULT_TIMEOUT', 0)
results_backend.set(key, zlib_compress(json_payload), cache_timeout)
results_backend.set(key, zlib_compress(
json_payload), cache_timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

here

- Add new attribute into Query model that keeps ID of an underlying
connection to database for this query
- Add `get_connection_id` method to EngineSpec that retrieves connection
id for a cursor.
- `execute_sql_statements` before executing queries retrieves and saves
connection id into Query
- Stop handler calls new `cancel_query` method of EngineSpec

Both new methods are implemented only for MySQL for now.
`cancel_query` uses KILL CONNECTION to terminate the statement.

Signed-off-by: Maxim Sukharev <[email protected]>
@smacker
Copy link
Contributor Author

smacker commented May 30, 2019

thanks! fixed & rebased on master

@smacker smacker merged commit 7fe2923 into src-d:master May 30, 2019
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.

2 participants