-
Notifications
You must be signed in to change notification settings - Fork 156
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
Clear out cache instead of rebuilding it when site data changes #829
Conversation
Tagged a couple different folks for review, but only needing one so whoever is able to first will suffice. |
$last_changed = get_site_option( 'last_changed_sites' ); | ||
|
||
if ( ! $last_changed ) { | ||
$last_changed = self::set_sites_last_changed_time(); |
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.
@dkotter In case of just-set $last_changed
, both delete_transient
functions will address non-existing transients, are we sure this is ok?
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.
@cadic I don't think it causes any issues to try deleting a transient that doesn't exist but I've modified this slightly so we only delete these transients if we have a proper last_changed_sites
value. Otherwise the transients should either not exist (so they don't need deleted) or they exist with a different last_changed_sites
value and will now be invalid since we've updated that value
… Otherwise we either don't have a proper transient to begin with or the transient will be invalid due to updating the last_changed_sites value, so no need to delete the transient
Description of the Change
In #355, changes were made on how available connection information is generated for individual users that dramatically improved performance. Among other things, this introduced caching around the available connections for a particular user. In order to ensure we have the most up-to-date connection list as possible, that cached data is rebuilt anytime site data is changed (like a new site added/deleted to the network) or a user gets added/removed from a site.
While there's nothing wrong with this, this can cause timeout issues in certain situations. For instance, if you have lots of sites on your network (75+) and you add a new site and at the same time add a couple dozen users to that site, those dozens of users will have their cache rebuilt, which requires iterating over all 75+ sites, which can lead to timeouts.
This PR attempts to fix this by clearing out the cached data instead of rebuilding it. This still ensures we have fresh data for all users but that regeneration process will now happen when a user first accesses their Push or Pull screen.
Alternate Designs
Initially considered trying to just regenerate the cache for the newly added site instead of all sites at once. But if the cache for a user is already expired (we only cache that data for 15 minutes so this is fairly likely) we would still have to loop over all sites, which doesn't solve the problem.
Benefits
There should now be less processing that happens when a new site is added to the network (or if some details about a site is changed)
Possible Drawbacks
We are now moving the cache regeneration process to when the Push or Pull screen is accessed instead of on site creation/site edit/user added hooks. So in the situation where a new user is added to a site and then that user immediately goes to see what sites they can Pull from, that data won't be cached yet and will be built at that point, whereas right now, that data would already be cached.
I'm not too worried about this because we have only been caching this data for 15 minutes, so that data expires pretty quickly and I'd guess in most situations, that data is already expired when a user goes to the Push or Pull screen and thus would need to be rebuilt anyway. So other than the situation of someone immediately going to a new site they've been added to, I don't think this realistically changes much in the cache process.
Verification Process
Checklist:
Changelog Entry