Skip to content
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

[FrameworkBundle] Added a keep-in-memory option for cache adapters #30984

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@pborreli
Copy link
Contributor

pborreli commented Apr 7, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no (option is false by default)
Deprecations? no
Tests pass? need tests
Fixed tickets N/A
License MIT
Doc PR symfony/symfony-docs#...

Adding a keep-in-memory option for cache adapters which is basically a shortcut using ChainAdapter with ArrayCache as first adapter.

@pborreli pborreli force-pushed the pborreli:cache-keep-in-memory branch from d6eae8a to 7177a42 Apr 7, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 7, 2019

@pborreli pborreli force-pushed the pborreli:cache-keep-in-memory branch from 7177a42 to ce51023 Apr 7, 2019

@pborreli pborreli force-pushed the pborreli:cache-keep-in-memory branch from ce51023 to 0cdfd92 Apr 7, 2019

@pborreli

This comment has been minimized.

Copy link
Contributor Author

pborreli commented Apr 7, 2019

@nicolas-grekas fixed your feedbacks, thanks

@@ -995,6 +995,7 @@ private function addCacheSection(ArrayNodeDefinition $rootNode)
->info('Overwrite the setting from the default provider for this adapter.')
->end()
->scalarNode('clearer')->end()
->booleanNode('keep_in_memory')->defaultFalse()->end()

This comment has been minimized.

Copy link
@OskarStark

OskarStark Apr 7, 2019

Contributor

Could you please add an info() tag?

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

this is missing an update in the corresponding xsd
don't forget opening a doc PR please
also, naming: keep-in-memory? enable-local-cache? any better idea?

}
unset($tags[0]['keep_in_memory']);
if (!empty($tags[0])) {
throw new InvalidArgumentException(sprintf('Invalid "%s" tag for service "%s": accepted attributes are "clearer", "provider", "name", "namespace", "default_lifetime" and "reset", found "%s".', $this->cachePoolTag, $id, implode('", "', array_keys($tags[0]))));

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 8, 2019

Member

the new attribute should be listed here

@nicolas-grekas nicolas-grekas changed the title [WIP] Added a keep-in-memory option for cache adapters [FrameworkBundle] Added a keep-in-memory option for cache adapters Apr 8, 2019

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Apr 8, 2019

Shouldn’t this feature be enabled by default, with an option to opt out?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 8, 2019

Shouldn’t this feature be enabled by default, with an option to opt out?

chaining can introduce subtle desynchronization effects because the upper pools don't know when the items in lower pools are invalidated. That's true for the remaining expiration time but also for tag-based invalidation.

An idea could be to default to using a 1s local cache when tags are not used.

This makes me think we should forbid using tags+keep-in-memory together, and that we should make keep-in-memory an integer, that would default to 1, which would set the number of seconds the local pool keeps items when the real expiration date is unknown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.