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.

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

Merged
merged 1 commit into from
Mar 7, 2018

Conversation

mistercrunch
Copy link
Member

No description provided.

@@ -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):
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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...

@codecov-io
Copy link

codecov-io commented Mar 6, 2018

Codecov Report

Merging #4543 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
superset/__init__.py 71.69% <100%> (+0.54%) ⬆️

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 2e780e4...4351bc9. Read the comment docs.

@hughhhh
Copy link
Member

hughhhh commented Mar 6, 2018

🚢

@john-bodley
Copy link
Member

LGTM

@mistercrunch mistercrunch merged commit d817b8d into apache:master Mar 7, 2018
@mistercrunch mistercrunch deleted the compression branch March 7, 2018 05:19
aqia358 pushed a commit to aqia358/incubator-superset that referenced this pull request Mar 8, 2018
@maver1ck
Copy link
Contributor

maver1ck commented Mar 13, 2018

@mistercrunch
How to disable this behaviour?
I have nginx in front of Superset for ssl and compression.

For sure it should be configuration option for that.
(it's kind of breaking change for many)

mistercrunch added a commit to lyft/incubator-superset that referenced this pull request Mar 13, 2018
@mistercrunch
Copy link
Member Author

mistercrunch commented Mar 14, 2018

@maver1ck oops, we should make that optional, though isn't nginx smart enough to not recompress if the payload is already compressed?

@mistercrunch
Copy link
Member Author

@maver1ck #4617

@maver1ck
Copy link
Contributor

maver1ck commented Mar 15, 2018

I'm thinking it's not. And when using nginx you can compress static files on hdd (this increase performance)
Thanks for the flag.

john-bodley pushed a commit to john-bodley/superset that referenced this pull request Apr 10, 2018
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.24.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.24.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants