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.

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

Memcache support, initially for tweets #211

Merged
merged 3 commits into from
May 14, 2017
Merged

Memcache support, initially for tweets #211

merged 3 commits into from
May 14, 2017

Conversation

johndrinkwater
Copy link
Member

This could be expanded to support: servers.php and stream.php.

Work is part of #208 to reduce our initial load time to visit the site, as the server stalls for a moment querying this API.

Requires server to have memcached installed, configured, and have our local creds.php updated to include: getMCHost() + getMCPort()

@johndrinkwater
Memcache support, initially for tweets
3f2af6b 
This could be expanded to support: servers.php and stream.php.
Work is part of SteamLUG#208
@johndrinkwater
Support memcache for servers.php
2cba8f0
@johndrinkwater
Avoid the else branch for memcache use, makes code clearer
67c59dc 
When access to memcache is offline, fetchOrStore will still return the
result of the function call; this makes the code more readable and
prevents copy/paste errors for the false branch. This mirrors the
behaviour added for the servers memcache update.

This does remove error reporting for memcache calls… because otherwise
passing in false here would produce errors in log for every visit.
@johndrinkwater
Copy link
Member Author

The expiration times I picked are 10 minutes for server data, 20 minutes for tweets.
If anyone has better ideas for those, suggestions would be great.

Thinking at least for the server page, we should make the viewer aware content is stale with an ‘updated * ago’

@Corben78
Copy link
Contributor

This PR works. memcached and php5-memcache (not php5-memcached doh) are installed and active on the server.
Tested this PR on staging with:

git fetch origin refs/pull/211/head:pr211
git checkout pr211

And deleted it afterwards again:

git checkout master
git branch -D pr211

Ready to merge.

Corben78
Corben78 approved these changes May 13, 2017
View reviewed changes
Copy link
Contributor

@Corben78 Corben78 left a comment

Choose a reason for hiding this comment

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

Tested ok.

@johndrinkwater
Copy link
Member Author

Lemme just check a few things, however watching Eurovision so it may take some minutes.

@johndrinkwater
Copy link
Member Author

A not very scientific comparison to say having this enabled is useful.

/news

Before http://www.webpagetest.org/result/170513_1R_Q88/
1336 ms First Byte Time
2.138s Document Complete

After (cached) http://www.webpagetest.org/result/170513_10_YDN/
826 ms First Byte Time
1.710s Document Complete

/servers

Before http://www.webpagetest.org/result/170513_CH_Y8E/
599 ms First Byte Time
4.443s Document Complete

After (cached) http://www.webpagetest.org/result/170513_6B_YBA/
635 ms First Byte Time
1.659s Document Complete

@johndrinkwater
Copy link
Member Author

Going to merge this in this state, stream work is not done but due to #212 we probably need to do some rethinking of that page before we do further work on it…

@johndrinkwater johndrinkwater merged commit 8686c9c into SteamLUG:master May 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Corben78 Corben78 Corben78 approved these changes

Assignees
No one assigned
Labels
enhancement priority:medium
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@johndrinkwater @Corben78