-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Setting up compression using flask-compress #4543
Conversation
superset/utils.py
Outdated
@@ -838,3 +843,38 @@ def get_filter_key(f): | |||
def get_update_perms_flag(): | |||
val = os.environ.get('SUPERSET_UPDATE_PERMS') | |||
return val.lower() not in ('0', 'false', 'no') if val else True | |||
|
|||
|
|||
def gzipped(f): |
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.
Max is this required? I felt that Flask-Compress
takes care of all of this. Also would it be possible to report the time it takes to execute the sql_json
and explore_json
endpoints w/ and w/o caching?
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.
Flask-Compress only takes care of static files :( , I agree that this seems in-scope for it, I'll open a PR with them in parallel to this.
I actually tested by looking at headers and the decorator does do the difference.
Looks like it's not py3 compatible, going to tweak this now.
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.
Actually ran more tests and you're right, I think the payload I used for testing didn't meet flask-compress threshold for zipping on my first round of tests...
ee28cac
to
4351bc9
Compare
Codecov Report
@@ Coverage Diff @@
## master #4543 +/- ##
==========================================
+ Coverage 71.12% 71.13% +<.01%
==========================================
Files 187 187
Lines 14780 14782 +2
Branches 1083 1083
==========================================
+ Hits 10513 10515 +2
Misses 4264 4264
Partials 3 3
Continue to review full report at Codecov.
|
🚢 |
LGTM |
@mistercrunch For sure it should be configuration option for that. |
(cherry picked from commit d817b8d)
@maver1ck oops, we should make that optional, though isn't nginx smart enough to not recompress if the payload is already compressed? |
I'm thinking it's not. And when using nginx you can compress static files on hdd (this increase performance) |
(cherry picked from commit d817b8d)
No description provided.