Added a first attempt for a readme #28
Conversation
| + protected function getWritableCacheDirectories(): array | ||
| + { | ||
| + return []; | ||
| + } |
There was a problem hiding this comment.
The problem with that is that you'll get write errors while using any of the symfony components that don't completely warm. If you don't log warnings that you won't notice... maybe it isn't important.
I addressed that here: #21 (comment)
and later remarked that it is more performant to not warm the pools directory at all and rather redirect the cache to a cold tmp folder: #21 (comment)
There was a problem hiding this comment.
Yeah, But I thought I did explain that you need to warm the cache before deployment. If you do, this is safe, right?
There was a problem hiding this comment.
Kind of. The pools cache.system (which writes to pools) is not completly warmed. Some symfony components will have a cold cache and will try to write and fail.
This will result in a warnings that you could ignore, they aren't fatal. But i'm not sure I would recommend such a setup.
There was a problem hiding this comment.
It sounds like that application has a suboptimal cache warming strategy. These warnings should be visible in Cloudwatch, right?
This is how I will run my applications for maximum performance. But it is optional (and not default), maybe this settings is not for everybody.
There was a problem hiding this comment.
And to the safe aspect: The pools directory is always optional. It can be completely empty and you application would work.
I can't really say which of these 3 scenarios is the best and should be recommended:
- not copying the pools folder and having all system pool writes silently fail (which happens even with warmed cache since not all caches are fully warmed). This isn't too bad as those entries are quickly generated but I'd like to somehow prevent the log spam here without suppressing all cache warnings.
- copying the pools folder (which takes time) but with the advantage that future request will be able to write
- not deploying the pools folder at all and reconfiguring it to
/tmp/pools= this was my final result in Symfony's capability to run on read-only out of the box #21 for being the easierst to setup... it doesn't even require the BrefKernel to work. And the overhead of having the pools folder cold is fairly small.
There was a problem hiding this comment.
I think your second scenario is the best default (as it currently is) and your first scenario is for optimal performance (what this PR is making a comment about).
There was a problem hiding this comment.
Thats the problem, we both "think" but we don't have hard evidence 😉
In 1 case your are right, option 2 is definitely the fastest if you optimizing for cold starts which is probably a good idea.
There was a problem hiding this comment.
I would add that it will generate warnings in your log like the ones described in symfony/symfony#29357 so that someone using that approach knows that this downside exists.
There was a problem hiding this comment.
Thats the problem, we both "think" but we don't have hard evidence
That is not true =)
In #18 I showed that symlinking in faster that copying. And symfony don't write to filesystem unless you doing filesystem caching. (docs).
We also already discussed that copying pools is a good default.
In 1 case your are right, option 2 is definitely the fastest if you optimizing for cold starts which is probably a good idea.
Yes, That is why I've documented exactly that.
The readme may not be complete after this PR but it is a good start and nothing in the PR is wrong. I would be happy to see contributions with more content, different deploy strategies or PRs with more logging. But that is all out of scope for this PR.
There was a problem hiding this comment.
Hi, I think I suffer from the same confusion here: I thought we decided on a different implementation.
Let's collect all the data we have and compare it. I have opened #31 for that. I'll be honest: we discussed so many things, with so many benchmark results, that I have trouble to keep track. Maybe you too ^^
|
I vote for merging this in the current state. The readme is correct related to the master branch. If we have intention to change the goals or method of this library later, those changes should also effect the readme. Im saying: Whatever the outcome of #31 might be is out of scope of this PR. The result of #31 should change the readme accordingly. |
|
@Nyholm agreed, that matches the current version of master. |
This will fix #1