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(trino): cancel_query doesn't abort query execution in Trino server #24913

Closed
wants to merge 1 commit into from

Conversation

dungdm93
Copy link
Contributor

@dungdm93 dungdm93 commented Aug 8, 2023

SUMMARY

Fix #24858

This PR has 2 parts: Superset part - this PR, Trino part: trinodb/trino-python-client#400

Waiting for trinodb/trino-python-client#400 merge first and I'll bump the trino version

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screencast.from.08-08-2023.16.17.37.webm

After:

Screencast.from.08-08-2023.16.07.09.webm

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Trino queries cannot be stopped in SQL Lab #24858
  • 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

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #24913 (53170ce) into master (85a7d5c) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

❗ Current head 53170ce differs from pull request most recent head 62c9778. Consider uploading reports for the commit 62c9778 to get more accurate results

@@            Coverage Diff             @@
##           master   #24913      +/-   ##
==========================================
- Coverage   69.00%   68.99%   -0.01%     
==========================================
  Files        1906     1906              
  Lines       74134    74144      +10     
  Branches     8208     8208              
==========================================
+ Hits        51153    51154       +1     
- Misses      20858    20867       +9     
  Partials     2123     2123              
Flag Coverage Δ
hive 54.16% <25.00%> (-0.01%) ⬇️
mysql 79.19% <25.00%> (-0.02%) ⬇️
postgres 79.29% <25.00%> (-0.02%) ⬇️
presto 54.06% <25.00%> (-0.01%) ⬇️
python 83.35% <25.00%> (-0.03%) ⬇️
sqlite 77.87% <25.00%> (-0.02%) ⬇️
unit 55.04% <25.00%> (-0.02%) ⬇️

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

Files Changed Coverage Δ
superset/db_engine_specs/trino.py 78.19% <25.00%> (-5.55%) ⬇️

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

@dungdm93 dungdm93 changed the title fix(trino): cancel_query doesn't abort query execution in Trino server [DNM] fix(trino): cancel_query doesn't abort query execution in Trino server Aug 8, 2023
@dungdm93 dungdm93 changed the title [DNM] fix(trino): cancel_query doesn't abort query execution in Trino server fix(trino): cancel_query doesn't abort query execution in Trino server Aug 8, 2023
@rusackas
Copy link
Member

The linked Trino PR still hasn't been merged, but the linked Superset Issue has been closed as resolved by #25680. Should we close this, or is there more to unpack here?

@giftig
Copy link
Contributor

giftig commented Aug 20, 2024

@rusackas we wrote #25680 to work around the trino client lib being blocking, but a more "ideal" solution would be to allow the trino client lib to not block and then handle it in a simpler way (this solution).

If these two PRs get to a state where they can be merged it's probably a neater solution. It looks like they've been abandoned though, so unless someone is invested in taking them over we can probably stick with the solution we already have.

@giftig
Copy link
Contributor

giftig commented Aug 24, 2024

I'll close this one per above, can always be reopened / adapted if changes go into the trino library to allow us simpler usage here.

@giftig giftig closed this Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trino queries cannot be stopped in SQL Lab
3 participants