[2.3] [WIP][HttpCache] StoreHouseKeepingInteface #6855
Conversation
| /** | ||
| * clear all the stale entries | ||
| * | ||
| * @abstract |
|
thanks. |
|
|
|
The script is an easy task as I said, but is correct if I create the script in the framework repo feedback here as well. |
|
What do you think @fabpot ? May I continue with the command and doc? |
|
@vicb ping |
|
I would prefer that this uses the yet to be defined CacheComponent (as @lsmith77 mention in a related PR). If this PR should be integrated as an interim solution - which I don't have the power to decide, you should clean the code according to the CS. |
|
fixed according with CS and squashed. |
| } | ||
|
|
||
| /** | ||
| * Deletes from disk all the stale response-header files and the response-content files |
|
If you need more info, they are at http://symfony.com/doc/current/contributing/index.html > CS / CC |
|
Many thanks... squashed again. |
| /** | ||
| * Deletes from disk all the stale response-header files and the response-content files. | ||
| * | ||
| * @return int The number of the cleared entries |
|
Squashed, all the comments are satisfied. |
| if (array_key_exists($key, $this->responseContentList)) { | ||
| return $this->responseContentList[$key] = $this->responseContentList[$key] || $need; | ||
| } | ||
| $this->responseContentList[$key] = $need; |
There was a problem hiding this comment.
👍
note: responseContentList 'd be intended as a white-list of contents, it should be true if at least one request need that response (is always possible that for the same response there are more request)
CS fixies little fix on a return value
|
In order to create the command, |
|
Don't know if I am "you" but I think a command could be great but the command should only makes use of a service (ie no intelligence in your command). this allows some other code to call your service. |
|
I mean a sf command |
|
"you" is anyone who wants to share his witticism, so you are perfect for this. |
| public function clear() | ||
| { | ||
| $finder = new Finder(); | ||
| $finder->depth('< 5')->files()->notName('*.lck')->in($this->root . DIRECTORY_SEPARATOR . 'md'); |
There was a problem hiding this comment.
using the finder looks problematic to me, it'd become very complicated to switch to the cache component or implement this in a custom store, the input data comes from a trick only possible with filesystem AFAIK.
There was a problem hiding this comment.
agree, what do you suggest?
There was a problem hiding this comment.
Actually here it'd be overkill, as it's an implementation relying on filesystem, there is no reason not to leverage an existing tool helping out.
I was just pointing out the fact that if the plan is to make HttpCache use the yet to come Cache component for 2.3, most of this PR would probably have to be rewritten anyway.
|
I'd also have a second remark, as metadata and cached content are independent (see #7361), there is also the other way round, cached content file not referenced by any metadata, this could happen when using validation, each time the page changes and becomes invalid, an other content file is created, and the old one is kept, it also happens when purging an url, the content file is kept. So I think the problem this PR is trying to solve (storage leaks) is unavoidable, unless it also browses all the cached content files path and removes the unreferenced ones (+ maybe other cases I can't think of), having a self cleaning cache store looks very complicated, imo the best solution remains to physically purge the storage it uses on a regular basis (here for example juste delete the whole cache folder). |
|
the goal of this PR is provide a command that on regular basis clear the
|
|
Are you sure it deletes unreferenced content files? I can't see any place where you're extracting content file paths, I might be missing something though. But let's forget about the implementation for a moment, actually I'm -1 for the feature in itself, personally I'd just delete the folder from time to time, I don't really see the need for this whole self-introspection. Looking at the original ticket, the reporter was saying he does a vary on cookies, I'm surprised nobody asked for more details about his use case, this is not common AFAIK, it probably leads to an entry creation for each user (assuming he's using sessions for example), then why using a shared cache? When the storage size is constantly growing, I think this feature would generally just hide bad practices or the fact that filesystem isn't the most appropriate storage for the website in question. IMO instead of introducing yet another interface heavily relying on filesystem, (I fail to see how it could be implemented with another storage driver), better would be to make the store agnostic about the way data is persisted (once the cache component will be ready), which would lead to a very powerful optimization, and then in a second step reconsider adding this feature in a more generic way. |
|
This PR has 2 months, it also depends from: RFC Cache component googlegroup |
|
any news? the feature sounds interesting to me |
|
Just leaving this for reference, now that we have nice Lock and Cache components :) You guys might like https://packagist.org/packages/toflar/psr6-symfony-http-cache-store :) |
|
I'm going to close this one. Re-reading my comment here (#4871 (comment)), I think we should not fix this issue. One could either do the cleanup out-of-band or switch to Varnish (which sounds like the better option). |
I started looking for a perfect name,
from https://www.varnish-software.com/static/book/Cache_invalidation.html#smart-bans,
but
BanLurkerInterfacedoesn't fit well and neitherCleanerInterface, andPurgerInterfaceneither as well.The original @fabpot 's idea was the best
StoreHousekeepingInterface.The concept is that HttpCache::clear() will call $this->store->clear();
then the creation of the command is an easy task. Any suggestion are always appreciated.
Todo: