-
Notifications
You must be signed in to change notification settings - Fork 14k
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: revert eTag cache feature for dashboard #11203
fix: revert eTag cache feature for dashboard #11203
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11203 +/- ##
==========================================
+ Coverage 65.60% 65.62% +0.02%
==========================================
Files 828 828
Lines 39167 39148 -19
Branches 3589 3589
==========================================
- Hits 25694 25690 -4
+ Misses 13361 13347 -14
+ Partials 112 111 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the revert! there's probably a way to surgically pull out some of @ktmud's change and leave it in, but it might be safer to just do the full revert here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes should be safe to most Superset deployments because one needs to manually turn on the ENABLE_DASHBOARD_ETAG_HEADER
feature for the code change to apply. But reverting is probably warranted since it's kind of expensive to have different ETag caches per user so maybe we can explore other paths to make the caching better for the dashboard page.
if you feel partial code is useful for something else, i suggest you create a new PR for the specific purpose. |
SUMMARY
After enable eTag header for dashboard request (in airbnb), we found the cached content has user_id and dashboard edit permission, etc, user-specific data, is shared by all the users.
Need to revert this feature (with 2 PRs) by now. Possible fix could be separate dashboard bootstrap data in the future.
TEST PLAN
CI
ADDITIONAL INFORMATION
cc @ktmud @etr2460 @john-bodley