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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Oct 13, 2023
Merged

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

merged 1 commit into from
Oct 13, 2023

Conversation

villebro
Copy link
Member

@villebro villebro commented Oct 5, 2023 •
edited
Loading

SUMMARY

This reverts commit cfda30c. The original PR tries to address a host of problems caused by this breaking change to the native Trino driver: trinodb/trino-python-client#220 (see changelog for 0.317: https://github.com/trinodb/trino-python-client/blob/master/CHANGES.md#breaking-changes-1). @dungdm93 is working on a fix for this, but it's not yet merged, and will likely require additional work on Superset before the issue is fully resolved: trinodb/trino-python-client#400. So reverting for now..

BEFORE

image

AFTER

Now Trino queries execute as expected:
image

TESTING INSTRUCTIONS

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 changed the title Revert "fix(sqllab): Force trino client async execution (#24859)" revert(sqllab): Force trino client async execution (#24859) Oct 5, 2023
@villebro villebro changed the title revert(sqllab): Force trino client async execution (#24859) revert: fix(sqllab): Force trino client async execution (#24859) Oct 5, 2023
@villebro villebro changed the title revert: fix(sqllab): Force trino client async execution (#24859) fix: revert fix(sqllab): Force trino client async execution (#24859) Oct 5, 2023
@villebro
Copy link
Member Author

villebro commented Oct 5, 2023

Ping @dungdm93 @giftig

michael-s-molina
michael-s-molina approved these changes Oct 5, 2023
View reviewed changes
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

@michael-s-molina 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 5, 2023
@michael-s-molina
Copy link
Member

Pinging @eschutho in case this PR needs to be included in 2.1

@villebro
Copy link
Member Author

villebro commented Oct 6, 2023

@michael-s-molina I'll leave this open until tomorrow PST if @giftig has something to add. Otherwise we'll just proceed with reverting and follow-up with a new PR to reintroduce the functionality and address the regression.

@giftig
Copy link
Contributor

giftig commented Oct 6, 2023 •
edited
Loading

So currently you're unable to execute trino queries at all? Do you have the full stacktrace of that 'query_id' error / did you get to the bottom of how the change has caused the error? This was tested with trino when originally raised but maybe it's interacted with some other changes in the 3.0 branch.

Happy to revert this for now if this is blocking a release but being able to stop a query is really important for trino where you might be running very intensive queries so let's catch up on what broke here so we can reinstate a new fix ahead of getting the library fix.

@michael-s-molina
Copy link
Member

@giftig @villebro I don't mind postponing 3.0.1 RC2 if we're able to submit a quick fix for the issue. If you think we can't fix this by the end of next week, then let's revert the change.

@villebro
Copy link
Member Author

villebro commented Oct 6, 2023

Quick update: we'll be discussing this with @giftig early next week. Optimally we'd prefer to fix this rather than revert, so if that's possible we'll do that. But if it seems risky we'll go ahead with the revert.

@michael-s-molina michael-s-molina merged commit e56e0de into apache:master Oct 13, 2023
37 of 42 checks passed
michael-s-molina pushed a commit that referenced this pull request Oct 13, 2023
@villebro @michael-s-molina
fix: revert fix(sqllab): Force trino client async execution (#24859) (#…
cd1b7a4 
…25541)

(cherry picked from commit e56e0de)
@giftig
Copy link
Contributor

giftig commented Oct 16, 2023 •
edited
Loading

Bear in mind that merging this PR fixes an issue which seems to only be reproducible on mac OS but re-introduces a bug which prevents trino queries being stopped across all environments. I'm currently blocked from amending this fix because I can't reproduce the issue after several attempts and it isn't clear what the scope of the bug is. I think it's important to understand that impact, because it's a major bug that's been reintroduced by reverting this fix.

@giftig
Copy link
Contributor

giftig commented Oct 16, 2023 •
edited
Loading

FYI I've made some progress and can now reproduce the bug; it's only broken when using sqlite as the metadata backend, but it's not mac-OS specific, I just had an issue in my method of reproducing the problem with sqlite. I'll do some digging and open a new PR reintroducing the fix in the next couple of days and patch the sqlite problem while I'm there.

@villebro villebro deleted the villebro/revert-trino-async branch October 16, 2023 17:08
@giftig
Copy link
Contributor

giftig commented Oct 17, 2023

PR to reinstate this fix with a very small amendment to default sqlite config: #25680

saghatelian added a commit to 10webio/superset that referenced this pull request Oct 23, 2023
@saghatelian @betodealmeida
@john-bodley @kgabryje @jfrag1 @michael-s-molina @justinpark @fisjac @dpgaspar @Always-prog @CorbinBullard @cnabro @celalettin1286 @villebro @nytai @mapledan @Khrol @zhaorui2022 @frassinier @hughhhh @OskarNS
3.0 (#2)
53bb43b 
* fix: is_select with UNION (apache#25290)

(cherry picked from commit bb002d6)

* fix: Add explicit ON DELETE CASCADE for dashboard_roles (apache#25320)

(cherry picked from commit d54e827)

* fix(chart): Supporting custom SQL as temporal x-axis column with filter (apache#25126)

Co-authored-by: Kamil Gabryjelski <[email protected]>

* fix: Use RLS clause instead of ID for cache key (apache#25229)

(cherry picked from commit fba66c6)

* fix: Improve the reliability of alerts & reports (apache#25239)

(cherry picked from commit f672d5d)

* fix: DashboardRoles cascade operation (apache#25349)

(cherry picked from commit a971a28)

* fix: datetime with timezone excel export (apache#25318)

Co-authored-by: Michael S. Molina <[email protected]>
(cherry picked from commit 5ebcd2a)

* fix: Workaround for Cypress ECONNRESET error (apache#25399)

(cherry picked from commit d76ff39)

* fix(sqllab): invalid persisted tab state (apache#25308) (apache#25398)

* fix: Rename on_delete parameter to ondelete (apache#25424)

(cherry picked from commit 893b45f)

* fix: preventing save button from flickering in SQL Lab (apache#25106)

(cherry picked from commit 296ff17)

* fix: chart import (apache#25425)

(cherry picked from commit a4d8f36)

* fix: swagger UI CSP error (apache#25368)

(cherry picked from commit 1716b9f)

* fix: smarter date formatter (apache#25404)

(cherry picked from commit f0080f9)

* fix(sqllab): invalid start date (apache#25437)

* fix(nativeFilters): Speed up native filters by removing unnecessary rerenders (apache#25282)

Co-authored-by: JUST.in DO IT <[email protected]>
(cherry picked from commit a0eeb4d)

* fix(SqlLab): make icon placement even (apache#25372)

(cherry picked from commit 11b49a6)

* fix: Duplicate items when pasting into Select (apache#25447)

(cherry picked from commit 7cf96cd)

* fix: update the SQLAlchemy model definition at json column for Log table (apache#25445)

(cherry picked from commit e83a76a)

* fix(helm chart): set chart appVersion to 3.0.0 (apache#25373)

* fix(mysql): handle string typed decimal results (apache#24241)

(cherry picked from commit 7eab59a)

* fix: Styles not loading because of faulty CSP setting (apache#25468)

(cherry picked from commit 0cebffd)

* fix(sqllab): error with lazy_gettext for tab titles (apache#25469)

(cherry picked from commit ddde178)

* fix: Address Mypy issue which is causing CI to fail (apache#25494)

(cherry picked from commit 36ed617)

* chore: Adds 3.0.1 CHANGELOG

* fix: Unable to sync columns when database or dataset name contains `+` (apache#25390)

(cherry picked from commit dbe0838)

* fix(sqllab): Broken query containing 'children' (apache#25490)

(cherry picked from commit b92957e)

* chore: Expand error detail on screencapture (apache#25519)

(cherry picked from commit ba541e8)

* fix: tags permissions error message (apache#25516)

(cherry picked from commit 50b0816)

* fix: Apply normalization to all dttm columns (apache#25147)

(cherry picked from commit 58fcd29)

* fix: REST API CSRF exempt list (apache#25590)

(cherry picked from commit 549abb5)

* fix(RLS): Fix Info Tooltip + Button Alignment on RLS Modal (apache#25400)

(cherry picked from commit a6d0e6f)

* fix: thubmnails loading - Talisman default config (apache#25486)

(cherry picked from commit 52f631a)

* fix(Presto): catch DatabaseError when testing Presto views (apache#25559)

Co-authored-by: Rui Zhao <[email protected]>
(cherry picked from commit be3714e)

* fix(Charts): Set max row limit + removed the option to use an empty row limit value (apache#25579)

(cherry picked from commit f556ef5)

* fix(window): unavailable localStorage and sessionStorage (apache#25599)

* fix: finestTemporalGrainFormatter (apache#25618)

(cherry picked from commit 62bffaf)

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

(cherry picked from commit e56e0de)

* chore: Updates 3.0.1 CHANGELOG

* fix(sqllab): Mistitled for new tab after rename (apache#25523)

(cherry picked from commit a520124)

* fix(sqllab): template validation error within comments (apache#25626)

(cherry picked from commit b370c66)

* fix: avoid 500 errors with SQLLAB_BACKEND_PERSISTENCE (apache#25553)

(cherry picked from commit 99f79f5)

* fix(import): Make sure query context is overwritten for overwriting imports (apache#25493)

(cherry picked from commit a0a0d80)

* fix: permalink save/overwrites in explore (apache#25112)

Co-authored-by: Elizabeth Thompson <[email protected]>
(cherry picked from commit e58a3ab)

* fix(header navlinks): link navlinks to path prefix (apache#25495)

(cherry picked from commit 51c56dd)

* fix: improve upload ZIP file validation (apache#25658)

* fix: warning of nth-child (apache#23638)

(cherry picked from commit 16cc089)

* fix(dremio): Fixes issue with Dremio SQL generation for Charts with Series Limit (apache#25657)

(cherry picked from commit be82657)

---------

Co-authored-by: Beto Dealmeida <[email protected]>
Co-authored-by: John Bodley <[email protected]>
Co-authored-by: Zef Lin <[email protected]>
Co-authored-by: Kamil Gabryjelski <[email protected]>
Co-authored-by: Jack Fragassi <[email protected]>
Co-authored-by: Michael S. Molina <[email protected]>
Co-authored-by: JUST.in DO IT <[email protected]>
Co-authored-by: Jack <[email protected]>
Co-authored-by: Daniel Vaz Gaspar <[email protected]>
Co-authored-by: Stepan <[email protected]>
Co-authored-by: Corbin Bullard <[email protected]>
Co-authored-by: Gyuil Han <[email protected]>
Co-authored-by: Celalettin Calis <[email protected]>
Co-authored-by: Ville Brofeldt <[email protected]>
Co-authored-by: ʈᵃᵢ <[email protected]>
Co-authored-by: Michael S. Molina <[email protected]>
Co-authored-by: mapledan <[email protected]>
Co-authored-by: Igor Khrol <[email protected]>
Co-authored-by: Rui Zhao <[email protected]>
Co-authored-by: Fabien <[email protected]>
Co-authored-by: Hugh A. Miles II <[email protected]>
Co-authored-by: OskarNS <[email protected]>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@villebro
fix: revert fix(sqllab): Force trino client async execution (apache#2…
5029afd 
…4859) (apache#25541)
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@villebro @sfirke
fix: revert fix(sqllab): Force trino client async execution (apache#2…
43408b0 
…4859) (apache#25541)
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@villebro
fix: revert fix(sqllab): Force trino client async execution (apache#2…
c40799e 
…4859) (apache#25541)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-s-molina michael-s-molina michael-s-molina approved these changes

@eschutho eschutho Awaiting requested review from eschutho

Assignees
No one assigned
Labels
size/L v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@villebro @michael-s-molina @giftig