Skip to content

Caching and compression for app store requests #21050

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.

Sign up for GitHub

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

Jump to bottom
Merged
MorrisJobke merged 2 commits into from
May 20, 2020
Merged

Caching and compression for app store requests #21050

MorrisJobke merged 2 commits into from
May 20, 2020

Conversation

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented May 19, 2020

While looking into our apps.nextcloud.com server I realized that the requests are not compressed. Even if the server supports it the clients don't request it. Thus the server sends out 5 MB instead of the compressed 2 MB. This and the raised cache time of 60 instead of 5 minutes should reduce the overall load of the app store server quite a lot.

As those are quite small changes I would backport them down to stable16.

@MorrisJobke MorrisJobke added bug 3. to review Waiting for reviews labels May 19, 2020
@MorrisJobke MorrisJobke added this to the Nextcloud 20 milestone May 19, 2020
@MorrisJobke
Copy link
Member Author

MorrisJobke commented May 19, 2020

/backport to stable19

@MorrisJobke
Copy link
Member Author

MorrisJobke commented May 19, 2020

/backport to stable18

@MorrisJobke
Copy link
Member Author

MorrisJobke commented May 19, 2020

/backport to stable17

@MorrisJobke
Copy link
Member Author

MorrisJobke commented May 19, 2020

/backport to stable16

kesselb
kesselb approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@rullzer
Copy link
Member

rullzer commented May 20, 2020

The tests say no

@MorrisJobke
Cache appstore requests for 60 instead of 5 minutes
3e0d8df 
Signed-off-by: Morris Jobke <[email protected]>
@MorrisJobke
Compress the appstore requests by default
6ffde12 
In test it reduced the transfered data from 5 MB to 2 MB. This should reduce the load on the appstore significantly.

Signed-off-by: Morris Jobke <[email protected]>
@MorrisJobke MorrisJobke force-pushed the bug/noid/caching-and-gzipping-of-appstore-requests branch from c64fd31 to 6ffde12 Compare May 20, 2020 07:51
@MorrisJobke
Copy link
Member Author

MorrisJobke commented May 20, 2020

The tests say no

I fixed them :) Wasn't as easy as a lot of logic was hidden in getTime values.

@ChristophWurst
Copy link
Member

ChristophWurst commented May 20, 2020

I wonder if this should be generally the default for our http client. Would there be any downside to that?

@rullzer
Copy link
Member

rullzer commented May 20, 2020

I wonder if this should be generally the default for our http client. Would there be any downside to that?

Not really actually. Saying that we support gzip is fine IMO

@MorrisJobke
Copy link
Member Author

MorrisJobke commented May 20, 2020

I wonder if this should be generally the default for our http client. Would there be any downside to that?

I would keep it like that for the backports and enable it for 20 in the default client?

@MorrisJobke
Copy link
Member Author

MorrisJobke commented May 20, 2020

CI is fine -> merge

@MorrisJobke MorrisJobke merged commit 526905c into master May 20, 2020
@MorrisJobke MorrisJobke deleted the bug/noid/caching-and-gzipping-of-appstore-requests branch May 20, 2020 09:05
This was referenced May 20, 2020
Merged
Merged
@MorrisJobke MorrisJobke mentioned this pull request May 20, 2020
Merged
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request May 20, 2020
Merged
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request May 20, 2020
Merged
@MorrisJobke
Copy link
Member Author

MorrisJobke commented May 20, 2020

I would keep it like that for the backports and enable it for 20 in the default client?

#21054

@pachulo pachulo mentioned this pull request Jun 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzer rullzer rullzer approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@kesselb kesselb kesselb approved these changes

@blizzz blizzz Awaiting requested review from blizzz

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug

Projects

None yet

Milestone

Nextcloud 20

Development

Successfully merging this pull request may close these issues.

5 participants

@MorrisJobke @rullzer @ChristophWurst @kesselb