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: url_params cache miss with global async query #23641

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

kekwan
Copy link
Contributor

@kekwan kekwan commented Apr 10, 2023

SUMMARY

Fixes initial cache miss when global async queries are used.

Currently, whenever a Jinja context is used in a query such as url_params with async queries enabled, a call to get_form_data is made. form_data which has the url_params is used to generate extra cache keys. Right now, form_data is empty as there is no request context in async queries. This means that we are always encountering a cache miss on the first query.

Since async queries won't have request context, we need to fallback to setting Flask global. https://github.com/apache/superset/blob/master/superset/views/utils.py#L182

BEFORE/AFTER SCREENSHOTS OR

BEFORE

Screen.Recording.2023-04-10.at.4.45.16.PM.mov

AFTER

Screen.Recording.2023-04-10.at.4.19.37.PM.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Jinja template is not working when async query is enabled #16650
  • 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

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for fixing this long standing bug!

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

@kekwan I was too quick to approve - there's a missing import, and some linting errors. Let's see if fixing those clears up CI

@kekwan kekwan force-pushed the url-param-dashboard-cache-fix branch from f6b31eb to fb60d4b Compare April 11, 2023 16:20
@kekwan kekwan force-pushed the url-param-dashboard-cache-fix branch from b4f5e4d to 3059f23 Compare April 11, 2023 16:27
@kekwan kekwan force-pushed the url-param-dashboard-cache-fix branch from cbe6899 to 6dbbad2 Compare April 11, 2023 16:35
@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #23641 (91a84fc) into master (95d71ff) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 91a84fc differs from pull request most recent head 6e0ff2b. Consider uploading reports for the commit 6e0ff2b to get more accurate results

@@           Coverage Diff           @@
##           master   #23641   +/-   ##
=======================================
  Coverage   68.08%   68.08%           
=======================================
  Files        1920     1920           
  Lines       73990    73992    +2     
  Branches     8091     8091           
=======================================
+ Hits        50374    50376    +2     
  Misses      21547    21547           
  Partials     2069     2069           
Flag Coverage Δ
hive 53.18% <66.66%> (-0.01%) ⬇️
mysql 79.21% <100.00%> (+<0.01%) ⬆️
postgres 79.29% <100.00%> (+<0.01%) ⬆️
presto 53.09% <66.66%> (-0.01%) ⬇️
python 83.14% <100.00%> (+<0.01%) ⬆️
sqlite 77.79% <100.00%> (+<0.01%) ⬆️
unit 53.02% <33.33%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset/charts/data/api.py 88.95% <100.00%> (+0.06%) ⬆️
superset/views/core.py 75.29% <100.00%> (+0.01%) ⬆️

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

@kekwan
Copy link
Contributor Author

kekwan commented Apr 11, 2023

thanks @villebro, fixed all the linting issues.

@villebro
Copy link
Member

@kekwan I restarted the Presto test suite - if it keeps coming back red we should consider disabling it in the Presto test suite (there's quite a few others that have had to be disabled, too)

@kekwan
Copy link
Contributor Author

kekwan commented Apr 11, 2023

@villebro seems like its been failing consistently on latest builds on master. let me know if you want me to disable it as part of this PR.

@villebro
Copy link
Member

@kekwan I believe it was fixed here: #23666 So a rebase should fix it

…orm_data

import flask g

fix linting

fix linting 2
@kekwan kekwan force-pushed the url-param-dashboard-cache-fix branch from 6dbbad2 to 6e0ff2b Compare April 13, 2023 16:55
@kekwan
Copy link
Contributor Author

kekwan commented Apr 13, 2023

thanks @villebro rebased

@villebro
Copy link
Member

Perfect, thanks for all the work and patience here @kekwan ! ❤️

@villebro villebro merged commit 19404bc into apache:master Apr 13, 2023
@kekwan kekwan deleted the url-param-dashboard-cache-fix branch April 13, 2023 22:28
sebastianliebscher pushed a commit to sebastianliebscher/superset that referenced this pull request Apr 28, 2023
@@ -299,6 +299,9 @@ def data_from_cache(self, cache_key: str) -> Response:
"""
try:
cached_data = self._load_query_context_form_from_cache(cache_key)
# Set form_data in Flask Global as it is used as a fallback
# for async queries with jinja context
setattr(g, "form_data", cached_data)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to do this instead of g.form_data = cached_data?

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 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/XS 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants