You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "kekwan (via GitHub)" <gi...@apache.org> on 2023/04/10 23:46:35 UTC
[GitHub] [superset] kekwan opened a new pull request, #23641: fix: url_params cache miss with global async query
kekwan opened a new pull request, #23641:
URL: https://github.com/apache/superset/pull/23641
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
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](https://github.com/apache/superset/blob/2.1/superset/jinja_context.py#L195) 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
https://user-images.githubusercontent.com/19199254/231020152-a95035bb-fd15-4f9c-adfe-53f44dbe3f7b.mov
AFTER
https://user-images.githubusercontent.com/19199254/231018854-2ad51ab0-dc99-44c9-8475-74908034f800.mov
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [X] Has associated issue: https://github.com/apache/superset/issues/16650
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #23641: fix: url_params cache miss with global async query
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #23641:
URL: https://github.com/apache/superset/pull/23641#discussion_r1162780681
##########
superset/charts/data/api.py:
##########
@@ -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)
Review Comment:
you appear to be missing an import
```python
from flask import g
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #23641: fix: url_params cache miss with global async query
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #23641:
URL: https://github.com/apache/superset/pull/23641#issuecomment-1503836611
@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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #23641: fix: url_params cache miss with global async query
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #23641:
URL: https://github.com/apache/superset/pull/23641#issuecomment-1507579284
Perfect, thanks for all the work and patience here @kekwan ! ❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kekwan commented on pull request #23641: fix: url_params cache miss with global async query
Posted by "kekwan (via GitHub)" <gi...@apache.org>.
kekwan commented on PR #23641:
URL: https://github.com/apache/superset/pull/23641#issuecomment-1507571641
thanks @villebro rebased
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kekwan commented on pull request #23641: fix: url_params cache miss with global async query
Posted by "kekwan (via GitHub)" <gi...@apache.org>.
kekwan commented on PR #23641:
URL: https://github.com/apache/superset/pull/23641#issuecomment-1503790756
thanks @villebro, fixed all the linting issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kekwan commented on pull request #23641: fix: url_params cache miss with global async query
Posted by "kekwan (via GitHub)" <gi...@apache.org>.
kekwan commented on PR #23641:
URL: https://github.com/apache/superset/pull/23641#issuecomment-1504103620
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kekwan commented on a diff in pull request #23641: fix: url_params cache miss with global async query
Posted by "kekwan (via GitHub)" <gi...@apache.org>.
kekwan commented on code in PR #23641:
URL: https://github.com/apache/superset/pull/23641#discussion_r1163059056
##########
superset/charts/data/api.py:
##########
@@ -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)
Review Comment:
thanks! missed that, was running a different version locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #23641: fix: url_params cache miss with global async query
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #23641:
URL: https://github.com/apache/superset/pull/23641
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a diff in pull request #23641: fix: url_params cache miss with global async query
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #23641:
URL: https://github.com/apache/superset/pull/23641#discussion_r1182929608
##########
superset/charts/data/api.py:
##########
@@ -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)
Review Comment:
Is there a reason to do this instead of `g.form_data = cached_data`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #23641: fix: url_params cache miss with global async query
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23641:
URL: https://github.com/apache/superset/pull/23641#issuecomment-1503763815
## [Codecov](https://codecov.io/gh/apache/superset/pull/23641?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23641](https://codecov.io/gh/apache/superset/pull/23641?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (95c943a) into [master](https://codecov.io/gh/apache/superset/commit/8bd827679116204aa523c3dd0487104d03ab7376?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8bd8276) will **decrease** coverage by `11.33%`.
> The diff coverage is `70.58%`.
> :exclamation: Current head 95c943a differs from pull request most recent head 6dbbad2. Consider uploading reports for the commit 6dbbad2 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23641 +/- ##
===========================================
- Coverage 67.92% 56.59% -11.33%
===========================================
Files 1918 1918
Lines 73882 73893 +11
Branches 8054 8058 +4
===========================================
- Hits 50183 41819 -8364
- Misses 21646 30015 +8369
- Partials 2053 2059 +6
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `53.16% <66.66%> (-0.01%)` | :arrow_down: |
| postgres | `?` | |
| python | `59.49% <66.66%> (-23.49%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `53.02% <33.33%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23641?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...tend/src/components/Chart/DrillBy/DrillByChart.tsx](https://codecov.io/gh/apache/superset/pull/23641?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS9EcmlsbEJ5Q2hhcnQudHN4) | `100.00% <ø> (ø)` | |
| [.../src/components/Chart/DrillBy/DrillByMenuItems.tsx](https://codecov.io/gh/apache/superset/pull/23641?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS9EcmlsbEJ5TWVudUl0ZW1zLnRzeA==) | `84.37% <0.00%> (-1.34%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/23641?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `36.16% <0.00%> (-38.90%)` | :arrow_down: |
| [...tend/src/components/Chart/DrillBy/DrillByModal.tsx](https://codecov.io/gh/apache/superset/pull/23641?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2hhcnQvRHJpbGxCeS9EcmlsbEJ5TW9kYWwudHN4) | `76.74% <76.92%> (-9.93%)` | :arrow_down: |
| [superset/charts/data/api.py](https://codecov.io/gh/apache/superset/pull/23641?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY2hhcnRzL2RhdGEvYXBpLnB5) | `88.95% <100.00%> (+0.06%)` | :arrow_up: |
... and [302 files with indirect coverage changes](https://codecov.io/gh/apache/superset/pull/23641/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #23641: fix: url_params cache miss with global async query
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #23641:
URL: https://github.com/apache/superset/pull/23641#issuecomment-1506535777
@kekwan I believe it was fixed here: #23666 So a rebase should fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org