Skip to content

[Cache] fix sharing cache between apps #35723

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Tobion
Copy link
Contributor

@Tobion Tobion commented Feb 14, 2020

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

The configurable cache prefix seed (#20610) does not give full control over the cache prefix because the container class is added to the prefix in any case. This is a problem because the container class contains the app env name. We use different app environments for different deployment targets (dev and test). We want dev and test to use the same redis cache. But this is impossible to achieve because even setting the cache prefix seed does not accomplish this.

@@ -52,8 +52,8 @@ public function process(ContainerBuilder $container)
$seed = '.'.$container->getParameterBag()->resolveValue($container->getParameter('cache.prefix.seed'));
} else {
$seed = '_'.$container->getParameter('kernel.project_dir');
$seed .= '.'.$container->getParameter('kernel.container_class');
Copy link
Contributor Author

@Tobion Tobion Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think we should set this combined seed as default in

->scalarNode('prefix_seed')
->info('Used to namespace cache keys when using several apps with the same shared backend')
->example('my-application-name')

Then the default behavior is obvious and people understand why they might want to change this value.

The configurable cache prefix seed does not give full control over the cache prefix because the container class is added to the prefix in any case. This is a problem because the container class contains the app env name. We use different app environments for different deployment targets (dev and test). Dev and test should use the same redis cache. But this is impossible to achieve because even setting the cache prefix seed does not accomplish this.
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 18, 2020

We want dev and test to use the same redis cache

I fear removing this parameter from the seed might create other kinds of problems.
Why do you want to share values for test and dev? Is that worth the risk?

@Tobion
Copy link
Contributor Author

Tobion commented Feb 20, 2020

Dev and test are meant to share some data caches and updating the cache on one should be reflected on the other. But this does not work currently. I think giving people the possibily to do something like this is important.
And forcing the seed to contain some random container class that has nothing to do with other caches in redis etc. seems kinda arbitrary and not comprehensible.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 24, 2020

Removing this parameter from the seed not only means dev and test will share the same storage, but also prod will.
This is not arbitrary: envs are separate instances of the app, there is no reason to share pools (by default at least).

@Tobion
Copy link
Contributor Author

Tobion commented Feb 24, 2020

Yes having it separated by default is fine. And it still is. This PR is about when people set the cache prefix seed manually so they actually have control over cache separation.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 24, 2020

Then this should be for master to me - we don't want to force-invalidate existing caches in a bug fix release.

We should also change https://github.com/symfony/recipes/blob/master/symfony/framework-bundle/3.3/config/packages/cache.yaml also:
prefix_seed: your_vendor_name/app_name/%kernel.environment%, so that ppl get a hint about the behavior and can easily opt-in to isolate their pools.

Note that I'm not getting the point of the change from a conceptual pov, but I'm still trying to be solution-oriented :)

@Tobion
Copy link
Contributor Author

Tobion commented Feb 25, 2020

Thanks for your consideration. I think it's important to be able to have full control over the cache prefix. E.g. if you try to mix symfony and a legacy app you likely also want to share caches. I think there are alot of valid use-cases.

I will do these changes in master then.

@Tobion
Copy link
Contributor Author

Tobion commented Feb 27, 2020

PR for master: #35890

@Tobion Tobion closed this Feb 27, 2020
@Tobion Tobion deleted the cache-prefix branch February 27, 2020 17:12
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
fabpot added a commit that referenced this pull request Sep 2, 2020
This PR was merged into the 5.2-dev branch.

Discussion
----------

[Cache] give control over cache prefix seed

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       |
| License       | MIT
| Doc PR        |

Reopened #35723 for master.

The configurable cache prefix seed does not give full control over the cache prefix because the container class is added to the prefix in any case. This is a problem because the container class contains the app env name. We use different app environments for different deployment targets (dev and test). We want dev and test to use the same redis cache. But this is impossible to achieve because even setting the cache prefix seed does not accomplish this.

Commits
-------

6681b92 [Cache] give control over cache prefix seed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants