fix(sqllab): reinstate "Force trino client async execution" #25680

giftig · 2023-10-17T15:17:51Z

SUMMARY

Reinstate the previous fix and adjust the default sqlite connection string to prevent the issue which caused us to revert it

SQLite seems to be a bit pedantic about multi-threaded access here, and seems to think we're sharing its objects between threads, though we haven't actually accessed the sqlite database from the extra thread.

We can prevent it being so pedantic with the check_same_thread=false option, so configure that by default with our basic development sqlite backend.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Run superset with the default sqlite3 as a backend and configure a trino database, e.g. by running the apache/superset docker image with trino driver added
Execute a query in trino

ADDITIONAL INFORMATION

Has associated issue: Trino backend fails with Superset 3.0.0 #25518, 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

villebro

LGTM, thanks for the fix! One small extra request about adding a note in the docs, as I think always having that param in SQLite connection strings IMO makes sense.

superset/config.py

Reinstate the previous fix and adjust the default sqlite connection string to prevent the issue causing us to revert it SQLite seems to be a bit pedantic about multi-threaded access here, and seems to think we're sharing its objects between threads, though we haven't actually accessed the sqlite database from the extra thread. We can prevent it being so pedantic with the check_same_thread=false option, so configure that by default with our basic development sqlite backend.

villebro

Tested to work as expected. Also, thanks for adding the docs!

…pache#25680)" This reverts commit 4e94fc4.

michael-s-molina · 2023-11-07T11:47:59Z

@sadpandajoe I noticed you reverted this PR in your branch. Did you find any problem with it?

sadpandajoe · 2023-11-07T17:11:25Z

@sadpandajoe I noticed you reverted this PR in your branch. Did you find any problem with it?

@michael-s-molina yeah we found a regression issue on our end where when we try to run a query it would just be stuck in loading and results never returned. We reverted it for now and we're debugging to see if it might be differences in drivers from our version versus superset's.

michael-s-molina · 2023-11-07T17:13:09Z

@michael-s-molina yeah we found a regression issue on our end where when we try to run a query it would just be stuck in loading and results never returned. We reverted it for now and we're debugging to see if it might be differences in drivers from our version versus superset's.

Thanks @sadpandajoe for the additional context. @justinpark this might be related to the problem we're seeing with multiple SQL statements.

…pache#25680)" This reverts commit 4e94fc4.

* fix(sqllab): reinstate "Force trino client async execution" (apache#25680) * fix: remove unnecessary redirect (apache#25679) (cherry picked from commit da42bf2) * fix(chore): dashboard requests to database equal the number of slices it has (apache#24709) (cherry picked from commit 75a7431) * fix: bump to FAB 4.3.9 remove CSP exception (apache#25712) (cherry picked from commit 8fb0c8d) * fix(horizontal filter label): show full tooltip with ellipsis (apache#25732) (cherry picked from commit e4173d9) * fix: Revert "fix(Charts): Set max row limit + removed the option to use an empty row limit value" (apache#25753) (cherry picked from commit e2fe967) * fix: dataset update uniqueness (apache#25756) (cherry picked from commit c7f8d11) * fix(sqllab): slow pop datasource query (apache#25741) (cherry picked from commit 2a2bc82) * fix: allow for backward compatible errors (apache#25640) * fix: DB-specific quoting in Jinja macro (apache#25779) (cherry picked from commit 5659c87) * fix: Revert "fix: Apply normalization to all dttm columns (apache#25147)" (apache#25801) * fix: Resolve issue apache#24195 (apache#25804) (cherry picked from commit 8737a8a) * fix(SQL field in edit dataset modal): display full sql query (apache#25768) (cherry picked from commit 1eba712) * fix(sqllab): infinite fetching status after results are landed (apache#25814) (cherry picked from commit 3f28eeb) * fix: Fires onChange when clearing all values of single select (apache#25853) (cherry picked from commit 8061d5c) * fix: the temporal x-axis results in a none time_range. (apache#25429) Co-authored-by: Elizabeth Thompson <[email protected]> (cherry picked from commit ae619b1) * fix(table chart): Show Cell Bars correctly apache#25625 (apache#25707) (cherry picked from commit 916f7bc) * fix: remove `update_charts_owners` (apache#25843) * fix(charts): Time grain is None when dataset uses Jinja (apache#25842) (cherry picked from commit 7536dd1) * fix: Saving Mixed Chart with dashboard filter applied breaks adhoc_filter_b (apache#25877) (cherry picked from commit 268c1dc) * fix: database version field (apache#25898) (cherry picked from commit 06ffcd2) * fix: trino cursor (apache#25897) (cherry picked from commit cdb18e0) * chore: Updates CHANGELOG.md for 3.0.2 * fix(trino): allow impersonate_user flag to be imported (apache#25872) Co-authored-by: John Bodley <[email protected]> (cherry picked from commit 458be8c) * fix(table): Double percenting ad-hoc percentage metrics (apache#25857) (cherry picked from commit 784a478) * fix(sqllab): invalid sanitization on comparison symbol (apache#25903) (cherry picked from commit 581d3c7) * fix: update flask-caching to avoid breaking redis cache, solves apache#25339 (apache#25947) Co-authored-by: Ville Brofeldt <[email protected]> * fix: always denorm column value before querying values (apache#25919) * chore(colors): Updating Airbnb brand colors (apache#23619) (cherry picked from commit 6d8424c) * fix: naming denomalized to denormalized in helpers.py (apache#25973) (cherry picked from commit 5def416) * fix(helm): Restart all related deployments when bootstrap script changed (apache#25703) * fix(rls): Update text from tables to datasets in RLS modal (apache#25997) (cherry picked from commit 210f1f8) * fix: Make Select component fire onChange listener when a selection is pasted in (apache#25993) (cherry picked from commit 5fccf67) * fix(explore): redandant force param (apache#25985) (cherry picked from commit e7a1876) * chore: Optimize fetching samples logic (apache#25995) (cherry picked from commit 326ac4a) * fix(native filters): rendering performance improvement by reduce overrendering (apache#25901) (cherry picked from commit e1d73d5) * fix: update FAB to 4.3.10, Azure user info fix (apache#26037) (cherry picked from commit 628cd34) * chore: Updates CHANGELOG.md for 3.0.2 (rc2) --------- Co-authored-by: Rob Moore <[email protected]> Co-authored-by: Igor Khrol <[email protected]> Co-authored-by: Stepan <[email protected]> Co-authored-by: Daniel Vaz Gaspar <[email protected]> Co-authored-by: Ross Mabbett <[email protected]> Co-authored-by: Geido <[email protected]> Co-authored-by: Beto Dealmeida <[email protected]> Co-authored-by: JUST.in DO IT <[email protected]> Co-authored-by: Elizabeth Thompson <[email protected]> Co-authored-by: John Bodley <[email protected]> Co-authored-by: Michael S. Molina <[email protected]> Co-authored-by: mapledan <[email protected]> Co-authored-by: Arko <[email protected]> Co-authored-by: Antonio Rivero <[email protected]> Co-authored-by: Kamil Gabryjelski <[email protected]> Co-authored-by: Michael S. Molina <[email protected]> Co-authored-by: FGrobelny <[email protected]> Co-authored-by: Giacomo Barone <[email protected]> Co-authored-by: Ville Brofeldt <[email protected]> Co-authored-by: Hugh A. Miles II <[email protected]> Co-authored-by: josedev-union <[email protected]> Co-authored-by: yousoph <[email protected]> Co-authored-by: Jack Fragassi <[email protected]>

…5680)

pull-request-size bot added the size/L label Oct 17, 2023

giftig force-pushed the issue-25518 branch from 147598d to 6bbe85a Compare October 17, 2023 15:25

This was referenced Oct 17, 2023

Trino backend fails with Superset 3.0.0 #25518

Closed

fix: revert fix(sqllab): Force trino client async execution (#24859) #25541

Merged

villebro approved these changes Oct 17, 2023

View reviewed changes

superset/config.py Show resolved Hide resolved

john-bodley added the review:checkpoint Last PR reviewed during the daily review standup label Oct 17, 2023

giftig force-pushed the issue-25518 branch from 6bbe85a to 6d4efca Compare October 18, 2023 10:15

john-bodley removed the review:checkpoint Last PR reviewed during the daily review standup label Oct 18, 2023

villebro approved these changes Oct 19, 2023

View reviewed changes

villebro merged commit 4e94fc4 into apache:master Oct 19, 2023
31 checks passed

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Oct 19, 2023

michael-s-molina pushed a commit that referenced this pull request Oct 31, 2023

fix(sqllab): reinstate "Force trino client async execution" (#25680)

5293f55

sadpandajoe added a commit to preset-io/superset that referenced this pull request Oct 31, 2023

Revert "fix(sqllab): reinstate "Force trino client async execution" (a…

678c48a

…pache#25680)" This reverts commit 4e94fc4.

sadpandajoe added a commit to preset-io/superset that referenced this pull request Oct 31, 2023

Revert "fix(sqllab): reinstate "Force trino client async execution" (a…

9ff15fd

…pache#25680)" This reverts commit 4e94fc4.

michael-s-molina added a commit to michael-s-molina/superset that referenced this pull request Nov 7, 2023

Revert "fix(sqllab): reinstate "Force trino client async execution" (a…

1544b8e

…pache#25680)" This reverts commit 4e94fc4.

michael-s-molina mentioned this pull request Nov 7, 2023

fix: Revert "fix(sqllab): reinstate "Force trino client async execution" (#25680) #25896

Closed

9 tasks

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

fix(sqllab): reinstate "Force trino client async execution" (apache#2…

3a6ccc8

…5680)

mistercrunch added 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

fix(sqllab): reinstate "Force trino client async execution" (apache#2…

ad950c7

…5680)

rusackas mentioned this pull request Aug 19, 2024

fix(trino): cancel_query doesn't abort query execution in Trino server #24913

Closed

9 tasks

villebro mentioned this pull request Sep 14, 2024

fix(examples): fix examples uri for sqlite #30277

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sqllab): reinstate "Force trino client async execution" #25680

fix(sqllab): reinstate "Force trino client async execution" #25680

giftig commented Oct 17, 2023

villebro left a comment

villebro left a comment

michael-s-molina commented Nov 7, 2023

sadpandajoe commented Nov 7, 2023

michael-s-molina commented Nov 7, 2023

fix(sqllab): reinstate "Force trino client async execution" #25680

fix(sqllab): reinstate "Force trino client async execution" #25680

Conversation

giftig commented Oct 17, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

villebro left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

michael-s-molina commented Nov 7, 2023

sadpandajoe commented Nov 7, 2023

michael-s-molina commented Nov 7, 2023