Cache invalidation #7319
Cache invalidation #7319
Conversation
betodealmeida
added
enhancement:request
| if (method === 'POST' && 'caches' in self) { | ||
| caches.open(CACHE_KEY).then(supersetCache => | ||
| supersetCache | ||
| .keys() |
There was a problem hiding this comment.
what exactly is in keys here? this is one req per key?
There was a problem hiding this comment.
keys are all the URLs stored in the cache.
Let's say you visited a dashboard that has a chart 1 and an extra filter name=dave. Your browser cache would look somewhat like this (using a pseudo-URL):
cache = {
'http://example.com/?slice_id=1&name=dave': Response(...),
}Now you go to the Explore view for that chart. When the page loads, it will do a GET and cache the request, so now your cache looks like this:
cache = {
'http://example.com/?slice_id=1&name=dave': Response(...),
'http://example.com/?slice_id=1': Response(...),
}You decide to change some parameters in chart 1. You click "Run query", which does a POST request. You save the chart.
Without cache invalidation, the following would happen:
- You visit the dashboard again. You notice the chart is different than what you saved, because you're seeing the cached version.
- You click "Force refresh". This does a
POSTrequest, and you see the new chart. You're happy. - You visit the dashboard again in the same day, before the cache expiration. You see the old chart again, because it's still cached in the browser and in the server. You're confused and sad.
With cache invalidation, the following will happen:
- You click "Run query" while exploring chart
1. - This invalidates all the responses stored on the client that reference that chart. This means your browser cache would now be empty.
- On the server side we can't iterate over keys, so only the
http://example.com/?slice_id=1response would be invalidated. - You save the chart.
- You visit the dashboard again. The browser cache is empty, so it does a
GETrequest for chart1with the extra filter (http://example.com/?slice_id=1&name=dave). - This is a cache hit on the server, so it returns old data.
- You click "Force refresh". This does a
POSTrequest, which returns the new chart and invalidateshttp://example.com/?slice_id=1&name=davein the server. You're happy. - You visit the dashboard again in the same day, before the cache expiration. The browser does a
GETrequest, misses caches, and shows the new chart. You're happy, and everything is cached now, both on the server and on the client.
There was a problem hiding this comment.
Thanks for the detailed response. Sorry if I'm misreading, but in the examples you gave above with ?slice_id=1 wouldn't this skip both of those keys because they do not contain form_data?
There was a problem hiding this comment.
@DiggidyDave, sorry for the confusion, I used a pseudo-URL in my examples to make it smaller. The full URL is actually /superset/explore_json/?form_data={"slice_id":1}.
Codecov Report
@@ Coverage Diff @@
## lyftga #7319 +/- ##
==========================================
+ Coverage 64.58% 64.67% +0.08%
==========================================
Files 425 426 +1
Lines 20884 20925 +41
Branches 2297 2305 +8
==========================================
+ Hits 13488 13533 +45
+ Misses 7270 7266 -4
Partials 126 126
Continue to review full report at Codecov.
|
|
Instead of end-to-end UI test for this, can we add some real unit tests and just mock the relevant storage/cache APIs? I think this needs some pretty robust testing, I'm afraid it is pretty tightly coupled to how URLs are structured and other things that could someday change and silently break this feature in an undiscoverable, hard-to-debug way. |
|
@DiggidyDave I'll do that. |
|
For cypress test, you can stubbing response to avoid hitting real Cache API call (and avoid |
|
Cypress is for end to end UX testing, though is it not? This is definitely much more suitable for fast, crisp unit tests. (e2e ux tests should be rare.) |
|
w.r.t. my concerns about coupling this with URL structure etc: will this be affected by issues @mistercrunch raised over here? Just wanted to make sure. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
CATEGORY
Choose one
SUMMARY
This PR implements cache invalidation for charts (after #7032), in two places:
(1) In the frontend, every time a
POSTis done bySupersetClientto get chart data, we invalidate the cache. This happens when the user clicks "force refresh" in a dashboard, or clicks "run query" in the explore view. When this happens, we extract the chart id (slice_id) and delete all stored responses that reference that chart — this would include, eg, a cached response of the chart with extra filters from a dashboard.(2) In the backend, we use a similar strategy. Unfortunately, we can't iterate over all the cached keys like in the frontend, to find all related cached responses. So we compute the key that would be generated if a
GETrequest was made, and invalidate it.TEST PLAN
I've tested manually, by modifying charts and ensuring that dashboards refresh correctly:
POSTrequest.GETand caches the response.POST, cache is invalidated for that chart. Server-side cache is also invalidated.Similarly for dashboards. Before, if you clicked force refresh on a dashboard it would performa a
POSTand show the new data. But reloading the dashboard would show the old charts, since they were still cached in the browser.I tried adding Cypress integration tests, but for some reason I get an error when writing to the Cache API (it works fine in Chrome) from an integration test:
ADDITIONAL INFORMATION
REVIEWERS