-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
[Config] Cache static resources #8370
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8370 +/- ##
==========================================
- Coverage 67.65% 60.55% -7.1%
==========================================
Files 448 332 -116
Lines 22498 10254 -12244
Branches 2364 2364
==========================================
- Hits 15220 6209 -9011
+ Misses 7140 3907 -3233
Partials 138 138 Continue to review full report at Codecov.
|
cc94271
to
681b522
Compare
681b522
to
ba4e385
Compare
ba4e385
to
48c379a
Compare
superset/views/core.py
Outdated
# HTTP_HEADERS is deprecated, this provides backwards compatibility | ||
override_http_headers.update(config.get("HTTP_HEADERS", {})) | ||
|
||
for k, v in override_http_headers.items(): |
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.
This could be defined as
response.headers.update(override_http_headers)
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.
done.
superset/views/core.py
Outdated
@@ -3127,8 +3127,17 @@ class CssTemplateAsyncModelView(CssTemplateModelView): | |||
@app.after_request | |||
def apply_http_headers(response): | |||
"""Applies the configuration's http headers to all responses""" | |||
for k, v in config.get("HTTP_HEADERS").items(): | |||
|
|||
override_http_headers = config.get("OVERRIDE_HTTP_HEADERS", {}) |
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.
Given that HTTP_HEADERS
and OVERRIDE_HTTP_HEADERS
are explicitly defined in the config I prefer not using get(...)
as this can be implied that the fields are optional. In Python 3 line #3131 and #3133 could be reduced to:
override_http_headers = {**config["OVERRIDE_HTTP_HEADERS"], **config["HTTP_HEADERS"]}
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.
HTTP_HEADERS will no longer be explicitly defined, but i'll update the other one
Didn't dig deep but this seems deterministic and looks like it can be done in module scope or cached somehow so that for every response we do a simple affectation (?) |
48c379a
to
a06e3fc
Compare
The joining of the dict probably could be cached, but not the application of default headers since different responses will have different headers applied. I don't think this is worth that optimization since these dicts are going to be quite small |
a06e3fc
to
450fde7
Compare
CATEGORY
Choose one
SUMMARY
Since static resources have hashed file names, we shouldn't need to even hit the webserver for them. Right now, we're hitting the server and using the etag to cache it, but this should prevent a trip to the server altogether.
It should make superset a bit more stable while deploying because these files won't be found on half the webservers while in the middle of deploying
This change also adds some flexibility to the http header configuration, allowing you to only set headers if they haven't already been set by superset. This means that we can cache static content with the
SEND_FILE_MAX_AGE_DEFAULT
var, but disallow caching on everything else withDEFAULT_HTTP_HEADERS = {"Cache-Control": "no-cache"}
.TEST PLAN
CI and a test deploy to see new cache control headers set
ADDITIONAL INFORMATION
REVIEWERS
@mistercrunch @dpgaspar @john-bodley