Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Make cache:clear "atomic" and consistent with cache:warmup#25117
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedNov 28, 2017
I like the goal (but didn't really look at the code yet.) I think this could target 3.4, as a bug fix. |
| $this->filesystem->rename($realCacheDir,$oldCacheDir); | ||
| if ('\\' ===DIRECTORY_SEPARATOR) { | ||
| sleep(1);// workaround for Windows PHP rename bug |
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.
This has been introduced in#8767, butlinks to a behavior that I'm not able to reproduce on PHP 5.5/7.1
I propose to remove it, since it slows down DX on Windows, and breaks "atomicity".
nicolas-grekas left a comment
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.
@hkdobrev FYI I merged my suggestion to your initial patch.
With this PR, the window for newly started concurrent requests to start with an empty container is very tiny, and the legacy container is also copied, so that already started concurrent requests can still use the legacy service factories.
This reuses the*.legacy files used byKernel, so these will be cleaned up in the end.
nicolas-grekas commentedDec 27, 2017
Re-qualified as bug on 3.4 btw. |
Taluu commentedDec 27, 2017
Wasn't the |
nicolas-grekas commentedDec 27, 2017
hkdobrev commentedDec 30, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nicolas-grekas Thanks for the review!
P.S. Or I just realised you've implemented that suggestion as well, I thought you were talking only about the Windows-specific thing. |
nicolas-grekas commentedDec 30, 2017
@hkdobrev that's already the case, I pushed on your branch. |
fabpot commentedDec 31, 2017
Thank you@hkdobrev. |
… with cache:warmup (hkdobrev)This PR was merged into the 3.4 branch.Discussion----------[FrameworkBundle] Make cache:clear "atomic" and consistent with cache:warmup| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes/no| Fixed tickets | -| License | MIT| Doc PR | -Here's what happens before/after this PR on `cache:clear` and `cache:clear --no-warmup`.### Before PR**`cache:clear`**1. `rm var/cache_old`1. Clearing cache in `var/cache` <i><--- This is not in line and not atomic</i>1. `rm var/cache_warmup`1. Warming up cache in `var/cache_warmup`1. `mv var/cache var/cache_old`1. `mv var/cache_warmup var/cache`1. `rm var/cache_old`**`cache:clear --no-warmup`**1. `rm var/cache_old`1. Clearing cache in `var/cache` <i><--- This is not in line and not atomic</i>1. `mv var/cache var/cache_old` <i><--- The old cache dir is completely obsolete in this workflow</i>1. `rm var/cache_old`---### After PR**`cache:clear`**1. `rm var/cache_old`1. `rm var/cache_new`1. Clearing cache in `var/cache_new`1. Warming up cache in `var/cache_new`1. `mv var/cache var/cache_old`1. `mv var/cache_new var/cache`1. `rm var/cache_old`**`cache:clear --no-warmup`**1. `rm var/cache_old`1. `rm var/cache_new`1. Clearing cache in `var/cache_new`1. `mv var/cache var/cache_old`1. `mv var/cache_new var/cache`1. `rm var/cache_old`---The main differences:- Unify the flows and have each distinct operation only once in the code- Clear the cache in the new cache directory and then switch to it atomically instead of clearing in the current one.- Always have the cache directory present in the end. It was missing after `cache:clear --no-warmup` before.I think this brings more consistency and is aligned with the present goals of the command as well.However, I'm not really familiar with the Symfony framework and I might have wrong assumptions.Commits-------8b88d9f [FrameworkBundle] Make cache:clear "atomic" and consistent with cache:warmup
Uh oh!
There was an error while loading.Please reload this page.
Here's what happens before/after this PR on
cache:clearandcache:clear --no-warmup.Before PR
cache:clearrm var/cache_oldvar/cache<--- This is not in line and not atomicrm var/cache_warmupvar/cache_warmupmv var/cache var/cache_oldmv var/cache_warmup var/cacherm var/cache_oldcache:clear --no-warmuprm var/cache_oldvar/cache<--- This is not in line and not atomicmv var/cache var/cache_old<--- The old cache dir is completely obsolete in this workflowrm var/cache_oldAfter PR
cache:clearrm var/cache_oldrm var/cache_newvar/cache_newvar/cache_newmv var/cache var/cache_oldmv var/cache_new var/cacherm var/cache_oldcache:clear --no-warmuprm var/cache_oldrm var/cache_newvar/cache_newmv var/cache var/cache_oldmv var/cache_new var/cacherm var/cache_oldThe main differences:
cache:clear --no-warmupbefore.I think this brings more consistency and is aligned with the present goals of the command as well.
However, I'm not really familiar with the Symfony framework and I might have wrong assumptions.