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: delete the correct dashboard cache key #11273

Merged
merged 2 commits into from
Oct 14, 2020
Merged

fix: delete the correct dashboard cache key #11273

merged 2 commits into from
Oct 14, 2020

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Oct 14, 2020 •
edited
Loading

SUMMARY

This PR fixes a bug from #11234 where cache keys for dashboard bootstrap data are incorrectly deleted.

We noticed this when some dashboard failed to update cache when updating charts.

delete_memoized from Flask caching treats instance method and class method differently:

        Delete memoized is also smart about instance methods vs class methods.
        When passing a instancemethod, it will only clear the cache related
        to that instance of that object. (object uniqueness can be overridden
        by defining the __repr__ method, such as user id).
        When passing a classmethod, it will clear all caches related across
        all instances of that class.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

  1. Create a dashboard and add a chart
  2. Change chart title in Dashboard edit mode
  3. Explore the chart, make updates and save
  4. Return to dashboard and refresh. In base, you may see the chart is not updated. After the fix, the chart should update.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@mistercrunch mistercrunch changed the title bugfix: delete dashboard cache key correctly fix: delete dashboard cache key correctly Oct 14, 2020
@ktmud ktmud changed the title fix: delete dashboard cache key correctly fix: delete the correct dashboard cache key Oct 14, 2020
etr2460
etr2460 reviewed Oct 14, 2020
View reviewed changes
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

a couple questions

superset/models/dashboard.py Outdated
@@ -294,7 +293,9 @@ def update_thumbnail(self) -> None:

@debounce(0.1)
def clear_cache(self) -> None:
cache.delete_memoized(self.full_data)
# pylint: disable=no-member
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to disable this lint error here?

superset/models/dashboard.py Outdated
@@ -252,8 +252,7 @@ def data(self) -> Dict[str, Any]:
}

@cache.memoize(
# manually maintain cache key version
make_name=lambda fname: f"{fname}-v1",
make_name=lambda fname: f"{fname}-v0.1",
Copy link
Member

Choose a reason for hiding this comment

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

why not v2?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess that works, too. Just thought this feature is still unstable so let's start with something small.

Copy link
Member

Choose a reason for hiding this comment

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

fair enough

@ktmud ktmud force-pushed the bugfix-dashboard-cache-cache-key branch from c8010b9 to 13b9039 Compare October 14, 2020 21:48
ktmud
ktmud commented Oct 14, 2020
View reviewed changes
superset/models/dashboard.py
@@ -294,7 +293,7 @@ def update_thumbnail(self) -> None:

@debounce(0.1)
def clear_cache(self) -> None:
cache.delete_memoized(self.full_data)
Copy link
Member Author

@ktmud ktmud Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The original code only updates the hash version related to the function: https://github.com/sh4nks/flask-caching/blob/v1.9.0/flask_caching/__init__.py#L1051-L1052

etr2460
etr2460 approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@etr2460 etr2460 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 fix!

superset/models/dashboard.py Outdated
@@ -252,8 +252,7 @@ def data(self) -> Dict[str, Any]:
}

@cache.memoize(
# manually maintain cache key version
make_name=lambda fname: f"{fname}-v1",
make_name=lambda fname: f"{fname}-v0.1",
Copy link
Member

Choose a reason for hiding this comment

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

fair enough

@ktmud
Copy link
Member Author

ktmud commented Oct 14, 2020

Updated the cache key nonetheless!

@codecov-io
Copy link

codecov-io commented Oct 14, 2020 •
edited
Loading

Codecov Report

Merging #11273 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11273      +/-   ##
==========================================
- Coverage   65.60%   65.60%   -0.01%     
==========================================
  Files         834      834              
  Lines       39566    39559       -7     
  Branches     3615     3610       -5     
==========================================
- Hits        25959    25954       -5     
+ Misses      13498    13496       -2     
  Partials      109      109
Flag Coverage Δ
#cypress 55.94% <ø> (+<0.01%) ⬆️
#javascript 62.71% <ø> (-0.04%) ⬇️
#python 60.78% <0.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
superset/models/dashboard.py 80.63% <0.00%> (ø)
...ontend/src/dashboard/components/dnd/handleHover.js 75.00% <0.00%> (-12.50%) ⬇️
...set-frontend/src/dashboard/util/getDropPosition.js 88.88% <0.00%> (-3.18%) ⬇️
...src/explore/components/controls/MetricsControl.jsx 89.69% <0.00%> (-0.19%) ⬇️
superset-frontend/src/components/Select/styles.tsx 98.14% <0.00%> (-0.13%) ⬇️
superset/queries/api.py 100.00% <0.00%> (ø)
superset/views/base_api.py 97.90% <0.00%> (+0.52%) ⬆️
...set-frontend/src/SqlLab/components/QuerySearch.jsx 59.22% <0.00%> (+1.67%) ⬆️
superset/db_engine_specs/postgres.py 100.00% <0.00%> (+2.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9caf875...ed9a7a0. Read the comment docs.

@etr2460 etr2460 merged commit 0262daa into apache:master Oct 14, 2020
etr2460 pushed a commit to airbnb/superset-fork that referenced this pull request Oct 14, 2020
@ktmud
fix: delete the correct dashboard cache key (apache#11273)
5caf0c9 
* bugfix: delete dashboard cache key correctly

* update version number
@ktmud ktmud deleted the bugfix-dashboard-cache-cache-key branch October 14, 2020 22:48
@ktmud ktmud mentioned this pull request Oct 15, 2020
Merged
6 tasks
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@ktmud @auxten
fix: delete the correct dashboard cache key (apache#11273)
aa479d0 
* bugfix: delete dashboard cache key correctly

* update version number
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etr2460 etr2460 etr2460 approved these changes

@graceguo-supercat graceguo-supercat Awaiting requested review from graceguo-supercat

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 1.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ktmud @codecov-io @etr2460 @mistercrunch