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

feat(trino): support early cancellation of queries #22498

Merged
merged 11 commits into from
Dec 24, 2022

Conversation

villebro
Copy link
Member

@villebro villebro commented Dec 21, 2022

SUMMARY

Currently cancelling queries on Trino fails if the query cancellation request is submitted before the query id has been retrieved. This typically happens when executing using async query execution, as the query first gets placed on the Celery queue prior to being executed. To work around this, we add a new method prepare_cancel_query on BaseEngineSpec which is called when calling the query cancellation endpoint. On Trino this will set an "early query cancellation" flag to True, which will trigger the query to cancel when BaseEngineSpec.handle_cursor is called during query execution. Appropriate tests are also added.

AFTER

Now stopping a query immediately shows the correct alert for both sync and async modes:

query-stop.mp4

BEFORE

sqlllab_before.mp4

In addition, when running queries in sync mode, cancelling a query would sometimes flash the following error:

image

This was due to the result handler not considering the possibility of the query being returned in a stopped state. To fix this, the query result handling logic is changed to only trigger if the result is successful. Some general type cleanup is also done.

TESTING INSTRUCTIONS

  1. Connect to a Trino database
  2. Execute a long running query and immediately cancel it
  3. Wait for an error to show up complaining about the query failing to stop.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@villebro villebro force-pushed the villebro/trino-cancel-early branch from 022f1c7 to 34b9148 Compare December 21, 2022 12:14
@codecov
Copy link

codecov bot commented Dec 21, 2022

Codecov Report

Merging #22498 (e9729a2) into master (9b26794) will decrease coverage by 0.01%.
The diff coverage is 60.46%.

@@            Coverage Diff             @@
##           master   #22498      +/-   ##
==========================================
- Coverage   66.90%   66.89%   -0.02%     
==========================================
  Files        1851     1851              
  Lines       70696    70715      +19     
  Branches     7764     7766       +2     
==========================================
+ Hits        47299    47303       +4     
- Misses      21375    21390      +15     
  Partials     2022     2022              
Flag Coverage Δ
hive 52.46% <54.16%> (-0.01%) ⬇️
javascript 53.85% <36.84%> (-0.01%) ⬇️
mysql 77.95% <54.16%> (-0.02%) ⬇️
postgres 78.02% <54.16%> (-0.02%) ⬇️
presto 52.35% <54.16%> (-0.01%) ⬇️
python 81.22% <79.16%> (-0.03%) ⬇️
sqlite ?
unit 51.18% <75.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/reducers/sqlLab.js 36.87% <0.00%> (ø)
...t-frontend/src/views/CRUD/data/query/QueryList.tsx 51.51% <0.00%> (-1.61%) ⬇️
superset/db_engine_specs/hive.py 87.40% <ø> (ø)
superset/db_engine_specs/presto.py 88.10% <ø> (ø)
superset/sql_lab.py 81.20% <33.33%> (-0.93%) ⬇️
...frontend/src/SqlLab/components/ResultSet/index.tsx 61.93% <66.66%> (ø)
superset/db_engine_specs/base.py 88.81% <85.71%> (-0.40%) ⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js 63.56% <100.00%> (+0.09%) ⬆️
...src/SqlLab/components/SqlEditorTabHeader/index.tsx 90.47% <100.00%> (ø)
superset/constants.py 100.00% <100.00%> (ø)
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@villebro villebro force-pushed the villebro/trino-cancel-early branch from bb0dfb0 to ed2f711 Compare December 22, 2022 07:21
@villebro villebro force-pushed the villebro/trino-cancel-early branch from ef32ab2 to 5f7b62d Compare December 23, 2022 07:20
@villebro villebro force-pushed the villebro/trino-cancel-early branch from 5f7b62d to 65f612c Compare December 23, 2022 07:23
@villebro villebro force-pushed the villebro/trino-cancel-early branch from ef4483d to 7e3977a Compare December 23, 2022 08:17
@zhaoyongjie zhaoyongjie self-requested a review December 24, 2022 02:17
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

I've not tested this PR in the production Trino cluster, but I think it works as expected and saves time for the slow queries.

@villebro villebro merged commit b6d39d1 into apache:master Dec 24, 2022
@villebro villebro deleted the villebro/trino-cancel-early branch December 24, 2022 04:31
john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Jan 25, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants