NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Cache][Lock] Add RedisProxy for lazy Redis connections#24887

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:3.4fromnicolas-grekas:lazy-redis

Nov 10, 2017

Merged

[Cache][Lock] Add RedisProxy for lazy Redis connections#24887

fabpot merged 1 commit intosymfony:3.4fromnicolas-grekas:lazy-redis

Nov 10, 2017

Conversation

Copy link

Member

nicolas-grekas commentedNov 9, 2017•
edited
Loading

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

That's the only provider that is not lazy by default, leading to bad DX (see linked issue.)
Best reviewedignoring whitespaces.

nicolas-grekas added this to the3.4 milestone

Nov 9, 2017

carsonbot added Status: Needs Review Cache Feature labels

Nov 9, 2017

Copy link

MemberAuthor

nicolas-grekas commentedNov 9, 2017

(now investigating the failure)

nicolas-grekas force-pushed thelazy-redis branch 3 times, most recently from778abd8 tofeca03fCompare

November 9, 2017 19:25

nicolas-grekas mentioned this pull request

Nov 9, 2017

Cache: RedisAdapter connects during cache:warmup (at least for Doctrine)#24865

Closed

Copy link

MemberAuthor

nicolas-grekas commentedNov 9, 2017

PR ready.

Copy link

Member

jderusse commentedNov 10, 2017

Same issue affects the Lock component, and could be patched in the same way. I will open a PR when this one will be merged to reuse the RedisProxy. (thanks FWBundle has a dependency on Cache)

Diff is to take into account and allow the new classRedisProxy inSymfony\Component\Lock\Store\RedisStore and inSymfony\Component\Lock\Store\StoreFactory and add thearray('lazy' => true) in theFrameworkExtension

[Cache][Lock] Add RedisProxy for lazy Redis connections

1f5e353

nicolas-grekas force-pushed thelazy-redis branch fromfeca03f to1f5e353Compare

November 10, 2017 07:19

nicolas-grekas changed the title~~[Cache] Add RedisProxy for lazy Redis connections~~[Cache][Lock] Add RedisProxy for lazy Redis connections

Nov 10, 2017

jderusse approved these changes

Nov 10, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 10, 2017

ro0NL reviewed

Nov 10, 2017

View reviewed changes

src/Symfony/Component/Cache/Traits/RedisTrait.php

		$this->enableVersioning();
		}elseif (!$redisClientinstanceof \Redis && !$redisClientinstanceof \RedisArray && !$redisClientinstanceof \Predis\Client) {
		}elseif (!$redisClientinstanceof \Redis && !$redisClientinstanceof \RedisArray && !$redisClientinstanceof \Predis\Client && !$redisClientinstanceof RedisProxy) {
		thrownewInvalidArgumentException(sprintf('%s() expects parameter 1 to be Redis, RedisArray, RedisCluster or Predis\Client, %s given',__METHOD__,is_object($redisClient) ?get_class($redisClient) :gettype($redisClient)));

Copy link

Contributor

ro0NLNov 10, 2017

Choose a reason for hiding this comment

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

this message needs an update right? Same for below.

Copy link

MemberAuthor

nicolas-grekasNov 10, 2017

Choose a reason for hiding this comment

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

I'd prefer not: RedisProxy is internal (same for docblocks)

Copy link

Contributor

ro0NLNov 10, 2017

Choose a reason for hiding this comment

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

right! 👍

chalasr approved these changes

Nov 10, 2017

View reviewed changes

ro0NL approved these changes

Nov 10, 2017

View reviewed changes

fabpot approved these changes

Nov 10, 2017

View reviewed changes

Copy link

Member

fabpot commentedNov 10, 2017

Thank you@nicolas-grekas.

fabpot merged commit1f5e353 intosymfony:3.4

Nov 10, 2017

fabpot added a commit that referenced this pull request

Nov 10, 2017

feature#24887[Cache][Lock] Add RedisProxy for lazy Redis connection…

bf52031

…s (nicolas-grekas)This PR was merged into the 3.4 branch.Discussion----------[Cache][Lock] Add RedisProxy for lazy Redis connections| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#24865| License       | MIT| Doc PR        | -That's the only provider that is not lazy by default, leading to bad DX (see linked issue.)Best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/24887/files?w=1).Commits-------1f5e353 [Cache][Lock] Add RedisProxy for lazy Redis connections

nicolas-grekas deleted the lazy-redis branch

November 10, 2017 15:26

This was referencedNov 12, 2017

Release v3.4.0-BETA4#24932

Merged

Release v4.0.0-BETA4#24933

Merged

Copy link

ahilles107 commentedNov 20, 2017•
edited
Loading

Hey, looks like it breaks Memcached adapter.

constant(): Couldn't find constant Memcached::OPT_LAZY--in MemcachedTrait.php (line 148)at MemcachedAdapter::Symfony\Component\Cache\Traits\{closure}(2,  'constant(): Couldn\'t find constant Memcached::OPT_LAZY',  '/Users/pawelmikolajczuk/Projects/asystentciazy-server/vendor/symfony/cache/Traits/MemcachedTrait.php',  148, array('servers' => array(array('127.0.0.1', 11211, 0)), 'options' => array('LAZY' => true, 'SERIALIZER' => 'php'), 'client' => object(Memcached), 'username' => null, 'password' => null, 'dsn' => 'memcached://127.0.0.1:11211', 'i' => 0, 'params' => array('scheme' => 'file', 'host' => '127.0.0.1', 'port' => 11211, 'weight' => 0), 'value' => true, 'name' => 'LAZY'))at constant('Memcached::OPT_LAZY')in MemcachedTrait.php (line 148)at MemcachedAdapter::createConnection(array(array('127.0.0.1', 11211, 0)), array('LAZY' => true, 'SERIALIZER' => 'php'))in AbstractAdapter.php (line 134)at AbstractAdapter::createConnection('memcached://127.0.0.1:11211', array('lazy' => true))in AppDevDebugProjectContainer.php (line 2444)

doctrine:    orm:        ...        metadata_cache_driver:            type: service            id: doctrine.system_cache_provider        query_cache_driver:            type: service            id: doctrine.system_cache_provider        result_cache_driver:            type: service            id: doctrine.result_cache_providerservices:    doctrine.result_cache_provider:        class: Symfony\Component\Cache\DoctrineProvider        public: false        arguments:            - '@doctrine.result_cache_pool'    doctrine.system_cache_provider:        class: Symfony\Component\Cache\DoctrineProvider        public: false        arguments:            - '@doctrine.system_cache_pool'framework:    cache:        default_memcached_provider: "memcached://127.0.0.1:11211"        pools:            doctrine.result_cache_pool:                adapter: cache.app            doctrine.system_cache_pool:                adapter: cache.adapter.memcached                public: true

Copy link

MemberAuthor

nicolas-grekas commentedNov 20, 2017

@ahilles107 correct, see#25038. I invite you to create issues when encountering an issue, as comments on closed PRs/issues are kinda lost most of the time. And looking at the history before doing so ;)