[WIP] Redis cache storage #3709

pdeszynski · 2013-02-07T13:20:49Z

New cache storage compatible with Redis. Heavily based on Memcached Storage.

marc-mabe · 2013-02-07T15:14:52Z

library/Zend/Cache/Storage/Adapter/Redis.php

+     * @param \Zend\Cache\Storage\Adapter\RedisOptions $options
+     * @see \Zend\Cache\Storage\Adapter\Abstract
+     */
+    public function __construct(RedisOptions $options = null)


This should be the first method.
$options can be array|Traversable|AbstractOptions -> remove the type-hint

marc-mabe · 2013-02-07T15:35:32Z

@piteer1 First, thanks for this PR - it is linked to #2582 but there are some points to solve:

see inline comments
I'm missing exceptions thrown
Please take a look into the current Memcached adapter. It has been changed to use a MemcachedResourceManager because one instance of the cache storage should only be used for one type of item. This means you have to share one resource over different instances if you like to store different type of items.
Your setUp() method of tests should switch against redis extension is loaded - see the method MemcachedTest::setUp()

PS: I'm not available until next Tuesday

pdeszynski · 2013-02-08T08:03:05Z

Hello,

Thank you for fast review. I did a mistake that I was basing on older version of memcached storage. I will try to make all the corrections during this weekend. I will make also RedisResourceManager to make it consistent with memcached storage.

Best regards

pdeszynski · 2013-02-11T09:12:18Z

Hello,

Generally I've updated the code to use a resource manager. I'm not too happy of the implementation yet.
For now setting password/database for Redis will cause reconnect to Redis Server. It should be changed to only call the correct method without reconnecting (or maybe it should not update RedisResource?).
It's also hard to say if given already initialized RedisResource to setResource was already connected or not. I'm checking right now if Redis->socket exists for that, but it's not really good approach.
Finally it should be reconsidered if not to use RedisArray for distributed connections, but I didn't have time to dig there yet.

Best regards

marc-mabe · 2013-02-12T11:03:04Z

library/Zend/Cache/Storage/Adapter/Redis.php

+    /**
+     * Set options.
+     *
+     * @param  array|Traversable|MemcachedOptions $options


MemcachedOptions -> RedisOptions

marc-mabe · 2013-02-12T11:22:47Z

@piteer1 Currently I can't test because of personal relocation.
For a static code analysis it looks good, only the missing previous exception information found and it would be good to have simple configuration methods within RedisOptions to configure the redis resource (To make static configuration simpler) - This can be found within MemcachedOptions already (not the methods marked as deprecated)

pdeszynski · 2013-02-12T11:26:58Z

@marc-mabe Still I think some changes are necessary. I'm not sure about some things #3709 (comment)

I will make later corrections according to your comments.

Freeaqingme · 2013-03-06T22:04:05Z

library/Zend/Cache/Storage/Adapter/Redis.php

+    /**
+     * Flushes all contents of current database
+     *
+     * @return bool Always true


Wouldn't null be a better return value if the return value is always the same?

@Freeaqingme That is described by FlushableInterface. It's because an operation could run into a failure throwing an exception but with the exception-handler plugin you could ignore such exception and return FALSE instead.
@piteer1 The phpdoc description isn't good

I've updated a little bit this phpdoc. According to https://github.com/nicolasff/phpredis#flushdb it's always true, I've removed Always true comment in @return

Freeaqingme · 2013-03-06T22:09:40Z

@piteer1 I really like where this PR is going. I have a couple of small issues:

It seems not everything was unit tested. I did a couple of random searches, and it seems setNamespace() and getDatabase() have not been tested (possibly other units haven't been either). Please review and update the tests where appropriate.
Would it be possible to implement a setter with a Redis resource? I can imagine that many people already have a Redis resource, and would like to simply inject it into this component.

Thanks!

marc-mabe · 2013-03-07T05:09:35Z

library/Zend/Cache/Storage/Adapter/Redis.php

+     *
+     * @return Redis
+     */
+    public function getRedisResource()


This should be protected - to get the redis resource the RedisResourceManager should be used

@return RedisResource

marc-mabe · 2013-03-07T05:11:47Z

@Freeaqingme You can inject an existing redis resource using RedisResourceManager::setResource()

pdeszynski · 2013-03-07T11:09:18Z

Thanks for the review, I will correct code later and answer all the questions.

pdeszynski · 2013-03-15T07:19:47Z

Redis converts everything to string. I think I misunderstood supportedDatatypes array. I've assumed that if the datatype is not preserverd it's not supported at all. I would like to claryfy things if it's possible. NULL, bool, integer and double will be automatically converted into string, so I've changed values of these to string. array, object, resource will be converted to array/object string, so valuses for these ones I've changed into false.
I'll be adding today some tests for RedisOptions/RedisResourceManager

pdeszynski · 2013-03-15T07:34:40Z

@Freeaqingme Yes it's like marc-mabe wrote, it's possible to set existing redis resource. RedisResourceManager will try to determine if redis resource is already connected or not. Sadly there will be a problem when existing redis resource is already connected and somebody will disconnect redis resource (directly on phpredis class). This way RedisResourceManager won't know how to reconnect to server, because it's not possible to get back connection info from phpredis object (server, password, database).

marc-mabe · 2013-03-15T08:23:54Z

@piteer1 The datatype array should tell the consumer how to handle different datatypes as follows:

true If the datatype is fully supported
- the value given on write is the same === as on read
string of type if the value get converted into another type or if the value is equal but not the same
- e.g. if an integer will be stored and returned as string it's integer => string
- e.g. if an integer will be serialized on write and unserialized on read and the value is the same it's integer => true
- e.g. if an object will be serialized on write and unserialized on read it's object => object

Could you create a future request for phpredis with a description why we need this and a link to this PR please.

marc-mabe · 2013-03-15T10:05:36Z

library/Zend/Cache/Storage/Adapter/Redis.php

+                        'double'   => 'string',
+                        'string'   => true,
+                        'array'    => 'string',
+                        'object'   => 'string',


I saw in you tests redis converts this into a string without any any serializion. So this is useless for customers and arrays and objects are not supported false

Yes, sorry, by mistake I've commited this. It should be of course false

…edis Storage tests

weierophinney · 2013-03-25T16:48:12Z

This is still marked as WIP -- @marc-mabe -- is it ready to review for inclusion yet?

pdeszynski · 2013-03-25T18:02:02Z

@marc-mabe I hope all issues were corrected. I've also added request for phpredis to add a possibility to obtain all settings back from phpredis class. If this would be implemented then I will refactor RedisResourceManager which will be much simplier then it's now.

marc-mabe · 2013-03-25T19:52:06Z

@weierophinney @piteer It looks great 👍

[WIP] Redis cache storage

Close #3709

…s-cache-storage [WIP] Redis cache storage

Close zendframework/zendframework#3709

marc-mabe reviewed Feb 7, 2013
View reviewed changes

Adapter for cache storage for Redis

41d5f69

Maks3w closed this Feb 8, 2013

Maks3w reopened this Feb 8, 2013

Updated redis to use resource manager

a7e23d5

marc-mabe reviewed Feb 12, 2013
View reviewed changes

ghost assigned weierophinney Feb 15, 2013

Freeaqingme reviewed Mar 6, 2013
View reviewed changes

marc-mabe reviewed Mar 7, 2013
View reviewed changes

Corrected 'supportedDatatypes' array, added some tests

9c9f80a

marc-mabe reviewed Mar 15, 2013
View reviewed changes

Corrected bug with setPersistentId function, added and removed some R…

23d8070

…edis Storage tests

pdeszynski mentioned this pull request Mar 25, 2013

Possibility to retrieve back all settings set to Redis class phpredis/phpredis#320

Closed

Added test of get/set database on RedisOptions

25503e6

weierophinney added a commit that referenced this pull request Mar 27, 2013

Merge pull request #3709 from piteer1/redis-cache-storage

e786950

[WIP] Redis cache storage

weierophinney added a commit that referenced this pull request Mar 27, 2013

Merge branch 'feature/3709' into develop

bbd5dab

Close #3709

weierophinney merged commit 25503e6 into zendframework:develop Mar 27, 2013

marc-mabe mentioned this pull request Mar 28, 2013

Implement Redis storage adapter #2582

Closed

pdeszynski deleted the redis-cache-storage branch April 3, 2013 20:29

weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#3709 from piteer1/redi…

8cdec41

…s-cache-storage [WIP] Redis cache storage

weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015

Merge branch 'feature/3709' into develop

71de0f7

Close zendframework/zendframework#3709

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Redis cache storage #3709

[WIP] Redis cache storage #3709

pdeszynski commented Feb 7, 2013

marc-mabe Feb 7, 2013

marc-mabe commented Feb 7, 2013

pdeszynski commented Feb 8, 2013

pdeszynski commented Feb 11, 2013

marc-mabe Feb 12, 2013

marc-mabe commented Feb 12, 2013

pdeszynski commented Feb 12, 2013

Freeaqingme Mar 6, 2013

marc-mabe Mar 7, 2013

pdeszynski Mar 15, 2013

Freeaqingme commented Mar 6, 2013

marc-mabe Mar 7, 2013

marc-mabe commented Mar 7, 2013

pdeszynski commented Mar 7, 2013

pdeszynski commented Mar 15, 2013

pdeszynski commented Mar 15, 2013

marc-mabe commented Mar 15, 2013

marc-mabe Mar 15, 2013

pdeszynski Mar 15, 2013

weierophinney commented Mar 25, 2013

pdeszynski commented Mar 25, 2013

marc-mabe commented Mar 25, 2013

[WIP] Redis cache storage #3709

[WIP] Redis cache storage #3709

Conversation

pdeszynski commented Feb 7, 2013

Choose a reason for hiding this comment

marc-mabe commented Feb 7, 2013

pdeszynski commented Feb 8, 2013

pdeszynski commented Feb 11, 2013

Choose a reason for hiding this comment

marc-mabe commented Feb 12, 2013

pdeszynski commented Feb 12, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freeaqingme commented Mar 6, 2013

Choose a reason for hiding this comment

marc-mabe commented Mar 7, 2013

pdeszynski commented Mar 7, 2013

pdeszynski commented Mar 15, 2013

pdeszynski commented Mar 15, 2013

marc-mabe commented Mar 15, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Mar 25, 2013

pdeszynski commented Mar 25, 2013

marc-mabe commented Mar 25, 2013