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(sqllab): non-blocking persistence mode #24539

Merged

Conversation

justinpark
Copy link
Member

SUMMARY

Following up SIP-93, this commit implements the first step of non-blocking persistence mode for the current working sqllab editor activities.

As described in SIP-93, the existing PERSISTENCE logic causes a user interaction delay due to the network (or server side) lagging. For example, user had to wait the server response in order to update the tab name or query limit count. (see the before screenshot)

Therefore, this commit aims to improve current tab activities by replacing the following blocking logics with a non-blocking way by cumulating the changes and syncing them with the latest updates asynchronously:

- querySuccess
- queryFailed
- toggleLeftBar
- queryEditorSetDb
- queryEditorSetSchema
- queryEditorSetAutorun
- queryEditorSetTitle
- queryEditorSetSql
- queryEditorSetQueryLimit
- queryEditorSetTemplateParams

Mechanism for syncing unsaved changes

debounced

There can be unsaved changes since it is syncing in a non-blocking way and asynchronously. To address this issue, unsaved changes will be stored in local storage (using localStorage in persistence mode) until they are successfully saved on the server. This hybrid approach ensures that user data is not lost and that any unsaved changes are retrieved, even in the event of an unexpected interruption.

Resolve conflicts

When a user opens multiple windows, they may be overwritten by the inactive editor tab state. To prevent this unexpected overwrite, the latest update time will be compared with the local cache state, and the out-of-updated state will be discarded. This ensures that users can work on multiple windows without losing their progress or having their work overwritten.

Following up

Creating new tabs and removing tabs are still blocking way in this commit but will be migrated into the non-blocking way in future PRs. Additionally, the existing migrateQueryEditorFromLocalStorage logic will be consolidated or replaced by the AutoSync with mutation APIs in the upcoming PRs. These changes will further improve the user experience by making the entire process non-blocking and asynchronous.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

after--nonblocking.mov

Before:

before--nonblocking.mov

TESTING INSTRUCTIONS

Set SQLLAB_BACKEND_PERSISTENCE true and play with the SQL Lab editor and keep hard refresh to verify the persisted states.

ADDITIONAL INFORMATION

  • Has associated issue: SIP-93
  • Required feature flags: SQLLAB_BACKEND_PERSISTENCE
  • 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

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.

Thanks for the PR @justinpark. I left some first-pass comments.

@michael-s-molina
Copy link
Member

@sadpandajoe @jinghua-qa could you help test the feature?

@sadpandajoe
Copy link
Member

@sadpandajoe @jinghua-qa could you help test the feature?

@michael-s-molina sure, if you want to spin up the ephemeral for me. I've noticed some issues already where results aren't always showing up so i want to make sure it's not an issue between OSS and Preset

@apache apache deleted a comment from github-actions bot Jul 12, 2023
@michael-s-molina
Copy link
Member

/testenv up FEATURE_SQLLAB_BACKEND_PERSISTENCE=true

@github-actions
Copy link
Contributor

@michael-s-molina Ephemeral environment spinning up at http://54.203.45.163:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@sadpandajoe
Copy link
Member

@justinpark looks like column auto-complete isn't working in this branch

Screen.Recording.2023-07-12.at.1.13.02.PM.mov

@sadpandajoe
Copy link
Member

sadpandajoe commented Jul 12, 2023

Table schemas get removed when switching between tabs

  • Open a first tab and fill out the relevant information and run a query
  • Open a new tab and select a different schema and run a query
  • Click back to the first tab

what happens?
The schema is removed from the first tab

Screen.Recording.2023-07-12.at.1.29.14.PM.mov

@sadpandajoe
Copy link
Member

User is not able to remove schemas without an error warning

  • Select a schema in the left dropdown
  • Click the remove button

What happens?
There is an error message to contact the administrator

@sadpandajoe
Copy link
Member

@justinpark is anything cached? I noticed that for columns it didn't show up the first time and then for another schema it works

@sadpandajoe
Copy link
Member

Results are not always shown with sql lab query. This is pretty hard to reproduce but also seem to be hitting a performance limit

  • Create 10 sql lab tabs
  • Run 10 different queries
  • Go to any other page
  • Go back to sql lab
  • Click on each tab and rerun the query

Around tab 6 i'm starting to see performance issues with that tab loading. You can also see at the end of the video that although there is data, it won't return any results.

Screen.Recording.2023-07-12.at.4.04.09.PM.mov

@justinpark justinpark force-pushed the feat--sqllab-non-blocking-persistence-mode branch from 9a696b1 to 7cb44c9 Compare July 13, 2023 23:22
@justinpark
Copy link
Member Author

@sadpandajoe Thanks for testing. The missing columns on autocomplete has been fixed from #24611 and I also rebased this branch to include the fix.
I also fixed the bug that is related to the tab switching with a new query editor. Could you try the test cases again?

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #24539 (cbc49cb) into master (1473d97) will decrease coverage by 0.06%.
The diff coverage is 87.28%.

❗ Current head cbc49cb differs from pull request most recent head a901a45. Consider uploading reports for the commit a901a45 to get more accurate results

@@            Coverage Diff             @@
##           master   #24539      +/-   ##
==========================================
- Coverage   69.03%   68.97%   -0.06%     
==========================================
  Files        1908     1903       -5     
  Lines       74197    74034     -163     
  Branches     8186     8190       +4     
==========================================
- Hits        51219    51068     -151     
+ Misses      20857    20845      -12     
  Partials     2121     2121              
Flag Coverage Δ
javascript 55.73% <80.64%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (ø)
...rontend/src/SqlLab/components/QueryTable/index.tsx 63.79% <0.00%> (ø)
superset-frontend/src/SqlLab/types.ts 57.14% <ø> (ø)
superset/reports/commands/exceptions.py 98.33% <ø> (-0.03%) ⬇️
superset/views/base.py 73.33% <ø> (ø)
superset/views/database/forms.py 93.67% <ø> (ø)
.../plugin-chart-pivot-table/src/plugin/buildQuery.ts 73.33% <50.00%> (+1.90%) ⬆️
superset/annotation_layers/api.py 86.20% <50.00%> (-0.12%) ⬇️
superset/css_templates/commands/delete.py 87.50% <50.00%> (ø)
superset/queries/saved_queries/commands/delete.py 87.50% <50.00%> (ø)
... and 35 more

... and 2 files with indirect coverage changes

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

@sadpandajoe
Copy link
Member

@justinpark @michael-s-molina do we need to spin up a new ephemeral for the changes?

@justinpark
Copy link
Member Author

/testenv up FEATURE_SQLLAB_BACKEND_PERSISTENCE=true

@github-actions
Copy link
Contributor

@justinpark Ephemeral environment creation is currently limited to committers.

@justinpark
Copy link
Member Author

@michael-s-molina could you help to boot up testenv?

@michael-s-molina
Copy link
Member

/testenv up FEATURE_SQLLAB_BACKEND_PERSISTENCE=true

@github-actions
Copy link
Contributor

@michael-s-molina Ephemeral environment spinning up at http://18.236.148.224:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@sadpandajoe
Copy link
Member

does ephemeral logins work for either of you @michael-s-molina / @justinpark? I haven't been testing since I can't log in.

@justinpark
Copy link
Member Author

@sadpandajoe same problem. @michael-s-molina have you faced same problem?

@justinpark justinpark force-pushed the feat--sqllab-non-blocking-persistence-mode branch from 3b9091b to b240338 Compare November 15, 2023 01:10
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

@justinpark justinpark merged commit e2bfb12 into apache:master Nov 20, 2023
26 checks passed
@justinpark justinpark deleted the feat--sqllab-non-blocking-persistence-mode branch November 20, 2023 19:13
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

josedev-union pushed a commit to Ortege-xyz/studio that referenced this pull request Jan 22, 2024
Co-authored-by: Justin Park <justinpark@apache.org>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
Co-authored-by: Justin Park <justinpark@apache.org>
@mistercrunch mistercrunch added 🏷️ 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
Co-authored-by: Justin Park <justinpark@apache.org>
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
Co-authored-by: Justin Park <justinpark@apache.org>
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/XXL 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants