-
Notifications
You must be signed in to change notification settings - Fork 800
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
Boost: add cache rebuild functionality #37151
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
* Any function that deletes, will now "invalidate" * When a post is trashed, cache files are still deleted. * Plugin actions will still delete cache files, not rebuild.
* Any delete_ functions become invalidate or rebuild * Add $action parameter to comment params * Fix comments so they say rebuild as well as delete.
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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.
I have proposed some suggestions/questions. Let me know what you think.
projects/plugins/boost/app/modules/optimizations/page-cache/pre-wordpress/Boost_Cache.php
Outdated
Show resolved
Hide resolved
projects/plugins/boost/app/modules/optimizations/page-cache/pre-wordpress/Boost_Cache.php
Outdated
Show resolved
Hide resolved
...cts/plugins/boost/app/modules/optimizations/page-cache/pre-wordpress/Boost_Cache_Actions.php
Outdated
Show resolved
Hide resolved
const DELETE_ALL = 'delete-all'; // delete all files and directories in a given directory, recursively. | ||
const DELETE_FILE = 'delete-single'; // delete a single file or recursively delete a single directory in a given directory. | ||
const DELETE_FILES = 'delete-files'; // delete all files in a given directory. | ||
const REBUILD_ALL = 'rebuild-all'; // rebuild all files and directories in a given directory, recursively. | ||
const REBUILD_FILE = 'rebuild-single'; // rebuild a single file or recursively rebuild a single directory in a given directory. | ||
const REBUILD_FILES = 'rebuild-files'; // rebuild all files in a given directory. |
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.
DELETE_ALL
and DELETE_FILE
appear to do the same thing. The same is true for REBUILD_ALL
and REBUILD_FILE
. Any chance we can combine each pair?
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.
FILE was supposed to be apply to a single file, but then got applied to a directory, which is what DELETE_ALL does. DELETE_FILE isn't used anywhere either, so these can definitely be simplified.
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.
I looked again at the code, and we need to keep DELETE_FILE. It's used when someone leaves a comment, and it's moderated, AND they clicked the "remember me" checkbox. We only want to delete the cache file for them, and nobody else. That bit of code used REBUILD_FILE, but since this file is unique to this user, there's no point rebuilding it. Nobody else will use it, so I replaced it with DELETE_FILE.
While it isn't used now, I'd like to keep REBUILD_FILE too, as it may well be used in the future.
...ts/plugins/boost/app/modules/optimizations/page-cache/pre-wordpress/storage/File_Storage.php
Show resolved
Hide resolved
…age is unique to them
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.
I've added a couple of suggestion. However, I tried to test this and faced some issues:
The rebuild doesn't work. Ones a page is updated, the next load is a miss
. I think this is happening because the file names aren't being compared correctly. Here is what I found in the directory:
index.html
68e7bda3bf0540bbcb4eaed73e14e98a.html
68e7bda3bf0540bbcb4eaed73e14e98a.html.rebuild.html.rebuild.html
projects/plugins/boost/app/modules/optimizations/page-cache/pre-wordpress/Boost_Cache.php
Outdated
Show resolved
Hide resolved
projects/plugins/boost/app/modules/optimizations/page-cache/pre-wordpress/Filesystem_Utils.php
Outdated
Show resolved
Hide resolved
…e-wordpress/Filesystem_Utils.php Fix this spelling. Make it easier to find this text. Co-authored-by: Adnan Haque <[email protected]>
…ck into add/boost/cache-rebuild
@haqadn I figured out why you were getting a cache miss on the rebuild file. The TTL was 10 seconds, but when a file is renamed, the filemtime isn't modified. I'll fix that. I fixed the rename too so it checks if it's renamed already. |
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.
Needs a trunk merge. But, works otherwise.
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.
Nice! Followed the instructions and it behaves as expected.
This PR adds the cache rebuild functionality from WP Super Cache to the cache in Boost. Instead of deleting expired files, they will be renamed, and if that page is requested, the cache file will be renamed back to its original name. The first visitor won't use that cache file, they'll generate a new one that overwrites the old one. Other visitors seeing the page will use that old cache page, until the first visitor generates a cache page.
There's a longer explanation in the issue #37054.
This PR differs from the description in the issue in one way. If the "rebuild file" is older than the cache TTL, the cache file is deleted and not used.
If a visitor resets a rebuild file and renames it back to the old cache file, the header sent will change from hit or miss to rebuild.
This feature is enabled by default in WPSC when site owners enable caching on the "easy" settings page, and this will be enabled by default in Boost as well. There is no UI to disable it.
The
rename()
commands added have to be silenced because two visitors may hit the site at the same time and go through the same code, but one will rename the file before the other.Should I add a constant to disable rebuild mode?
Fixes #37054
Proposed changes:
delete_directory
function has been renamedwalk_directory
because it handles rebuilding files too.delete_expired_files
function has been renamed togc_expired_files
and has an extra parameter,$action
that tells it to delete or rebuild files.rebuild_file
andrestore_file
functions to handle the renaming.reset_rebuild_file
function to check if rebuild file is there, the age, and renaming it back.garbage_collect
function now callsdelete_expired_files
with the action set to REBUILD.invalidate
function was updated to support rebuilding.Other information:
Jetpack product discussion
pc9hqz-2QH-p2
Does this pull request change what data or activity we track or use?
no
Testing instructions:
wget
orcurl
commands with "&" to put them in the background.