Skip to content

[FrameworkBundle] Allow creating chained cache pools by providing several adapters #32294

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

Merged
merged 1 commit into from
Jul 5, 2019

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jun 30, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Replaces #30984, follows symfony/symfony-docs#11813

This PR allows defining several adapters for one pool. When doing so, this defines a chain pool.
The benefit is that all chained pools get automatic namespace and lifetime, so things are transparent:

pools:
    my_chained_pool:
        default_lifetime: 12
        adapters:
          - cache.adapter.array
          - cache.adapter.filesystem
          - {name: cache.adapter.redis, provider: 'redis://foo'}

(see fixtures for example of PHP/XML config)

/cc @Nyholm @pborreli FYI

@Nyholm
Copy link
Member

Nyholm commented Jun 30, 2019

Thank you for this PR. It works as expected =)

I like this way more because we only use one traceable adapter. See cache.array, cache.apcu and cache.redis.

Before

Screenshot 2019-06-30 at 18 43 55

After

Screenshot 2019-06-30 at 18 43 48

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jun 30, 2019

For reference, this leverages a new convention supported by the cache.pool tag:
When a service is defined with the cache.pool tag,
and when the class of this service is ChainAdapter
then the first argument of this service must be defined as a list of strings: the adapters to chain.
The keys can optionally be used to define the provider for each adapter:

my_chain_cache:
    class: Symony\Component\Cache\Adapter\ChainAdapter
    arguments:
        0: cache.adapter.array
        redis://foo: cache.adapter.redis

This is a bit strange I agree, yet this should be pretty rare - it's mostly for bundles that couldn't use the fwb configuration.
The CachePoolPass will then turn this list to a proper list of adapter services, with their namespace/lifetime/etc properly configured, as really needed by ChainAdapter.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jun 30, 2019

For reference too, it should be possible to add a local memory cache to all "app" pools:

framework:
    cache:
        app: cache.app.chain
        pools:
            api_cache: ~ # "api_cache" pool uses "cache.app" as template, which is a chain
            cache.app.chain:
                adapters:
                  - cache.adapter.array
                  - cache.adapter.filesystem # or any other backend

(this is combining knowledge that the doc already gives - or will after symfony/symfony-docs#11855)

/cc @andrerom you might like this too :)

@andrerom
Copy link
Contributor

andrerom commented Jul 3, 2019

I like it

But if you plan to expose use of ArrayAdapter to a wider audience you should probably also add the following features to it:

  • max item limit to avoid reaching memory limits
  • own (very low, like default 100-500ms) TTL for in-memory caching, as it's in practice stale data when used in concurrent scenarios

If you want to be advance you can also:

  • keep track of use, and evict cache items based on that using LFU when reaching limit
  • in-memory cache is domain & project specific in terms of how long it's somewhat "safe" to keep items in memory, so either describe when to use and not use on a per pool term, or allow use of pool to pass in flags to opt out of in-memory cache for cases developer knows it should be ignored

We opted to not do this as a PSR Pool in eZ Platform 2.5 precisely for the last reason here, some data domains has a in-memory TTL of a few seconds, others just some hundred milliseconds.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 3, 2019

Thank you for the ideas @andrerom
For now, I've added the "reset" tag on chain adapters, so that each request always starts with an empty array pool. Should be enough for a 1st iteration, don't you think?

@andrerom
Copy link
Contributor

andrerom commented Jul 4, 2019

so that each request always starts with an empty array pool

Normally array pool will be empty on each new request or command execution right? Or are you talking about ReactPHP or future 7.4's pre-loading scenarios?

For normal fpm setup the start of the request is not an issue, it's for instance when something is long lived it becomes increasingly an issue, request or command.

That said, as long as doc does not use array as an example you can move ahead with this and we can try to make it more safe to use before 4.4 is out in order to document usage of it (including full pros/cons).

@nicolas-grekas
Copy link
Member Author

are you talking about ReactPHP or future 7.4's pre-loading scenarios?

yes, that's what I'm referring to. I've actually submitted #32361 to reset array pools, this was forgotten.

But I agree, let's deal with these concerns separately.

PR is ready.

nicolas-grekas added a commit that referenced this pull request Jul 4, 2019
…nsionConfigurationPass (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] fix processing of regular parameter bags by MergeExtensionConfigurationPass

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Spotted in and needed by #32294

Commits
-------

b06d000 [DI] fix processing of regular parameter bags by MergeExtensionConfigurationPass
@fabpot
Copy link
Member

fabpot commented Jul 5, 2019

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 29ba091 into symfony:4.4 Jul 5, 2019
fabpot added a commit that referenced this pull request Jul 5, 2019
…y providing several adapters (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[FrameworkBundle] Allow creating chained cache pools by providing several adapters

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Replaces #30984, follows symfony/symfony-docs#11813

This PR allows defining several adapters for one pool. When doing so, this defines a chain pool.
The benefit is that all chained pools get automatic namespace and lifetime, so things are transparent:

```yaml
pools:
    my_chained_pool:
        default_lifetime: 12
        adapters:
          - cache.adapter.array
          - cache.adapter.filesystem
          - {name: cache.adapter.redis, provider: 'redis://foo'}
```

(see fixtures for example of PHP/XML config)

/cc @Nyholm @pborreli FYI

Commits
-------

29ba091 [FrameworkBundle] Allow creating chained cache pools by providing several adapters
@nicolas-grekas nicolas-grekas deleted the fwb-adapter-chain branch July 19, 2019 11:11
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Jul 30, 2019
…(Nyholm)

This PR was submitted for the master branch but it was squashed and merged into the 4.4 branch instead (closes #11855).

Discussion
----------

Document how to create cache pool with framework config

This is some docs for symfony/symfony#32294

I removed the "old" way of doing it because this is far better.

Commits
-------

b0dc28f Document how to create cache pool with framework config
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
nicolas-grekas added a commit that referenced this pull request Jan 27, 2020
…che (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Cache] Add LRU + max-lifetime capabilities to ArrayCache

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix https://github.com/orgs/symfony/projects/1#card-30686676
| License       | MIT
| Doc PR        | -

In #32294 (comment), @andrerom writes:

> if you plan to expose use of ArrayAdapter to a wider audience you should probably also add the following features to it:
> - max item limit to avoid reaching memory limits
> - own (very low, like default 100-500ms) TTL for in-memory caching, as it's in practice stale data when used in concurrent scenarios
>
> If you want to be advance you can also:
>
> - keep track of use, and evict cache items based on that using LFU when reaching limit
> - in-memory cache is domain & project specific in terms of how long it's somewhat "safe" to keep items in memory, so either describe when to use and not use on a per pool term, or allow use of pool to pass in flags to opt out of in-memory cache for cases developer knows it should be ignored

This PR implements these suggestions, via two new constructor arguments: `$maxLifetime` and `$maxItems`.

In Yaml:
```yaml
services:
    app.lru150_cache:
        parent: cache.adapter.array
        arguments:
            $maxItems: 150
            $maxLifetime: 0.150

framework:
    cache:
        pools:
            my_chained_pool:
                adapters:
                  - app.lru150_cache
                  - cache.adapter.filesystem
```

This configuration adds a local memory cache that keeps max 150 elements for 150ms on top of a filesystem cache.

/cc @lyrixx since you were also interested in it.

Commits
-------

48a5d5e [Cache] Add LRU + max-lifetime capabilities to ArrayCache
symfony-splitter pushed a commit to symfony/cache that referenced this pull request Jan 27, 2020
…che (nicolas-grekas)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[Cache] Add LRU + max-lifetime capabilities to ArrayCache

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix https://github.com/orgs/symfony/projects/1#card-30686676
| License       | MIT
| Doc PR        | -

In symfony/symfony#32294 (comment), @andrerom writes:

> if you plan to expose use of ArrayAdapter to a wider audience you should probably also add the following features to it:
> - max item limit to avoid reaching memory limits
> - own (very low, like default 100-500ms) TTL for in-memory caching, as it's in practice stale data when used in concurrent scenarios
>
> If you want to be advance you can also:
>
> - keep track of use, and evict cache items based on that using LFU when reaching limit
> - in-memory cache is domain & project specific in terms of how long it's somewhat "safe" to keep items in memory, so either describe when to use and not use on a per pool term, or allow use of pool to pass in flags to opt out of in-memory cache for cases developer knows it should be ignored

This PR implements these suggestions, via two new constructor arguments: `$maxLifetime` and `$maxItems`.

In Yaml:
```yaml
services:
    app.lru150_cache:
        parent: cache.adapter.array
        arguments:
            $maxItems: 150
            $maxLifetime: 0.150

framework:
    cache:
        pools:
            my_chained_pool:
                adapters:
                  - app.lru150_cache
                  - cache.adapter.filesystem
```

This configuration adds a local memory cache that keeps max 150 elements for 150ms on top of a filesystem cache.

/cc @lyrixx since you were also interested in it.

Commits
-------

48a5d5e8a9 [Cache] Add LRU + max-lifetime capabilities to ArrayCache
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.

5 participants