Added ShardablePhpRedis backend #5

pounard · 2015-04-18T13:51:47Z

Added ShardablePhpRedis backend for cache, fixed minor details.

This pull request is now complete, please I'd like some feedback.

@Berdir Once we agree on the majority of points and once you merge, I'll copy your repo over the drupal.org project and give you the owner rights for you to be able to maintain the 8.x branch if you wish to.

pounard · 2015-04-18T13:52:43Z

redis.services.yml

@@ -4,3 +4,6 @@ services:
    arguments: ['@redis.factory', '@cache_tags.invalidator.checksum']
  redis.factory:
    class: Drupal\redis\ClientFactory
+  redis.phpredis.invalidator:
+    class: Drupal\redis\Cache\RedisCacheTagsChecksum
+    arguments: ['@redis.factory']


I am not really sure why I wrote this, I had trouble registering the checksum invalidator into services, @Berdir please tell me if this right or if I need to remove it.

pounard · 2015-04-19T10:21:32Z

src/Cache/RedisCacheTagsChecksum.php

+        // invalid, but that's the definitive price of trying to being
+        // consistent in all cases.
+        $current = $this->getNextIncrement();
+        $this->client->set($this->getKey(['tag', $tag]), $current);


Note that I want to restore the tag cache around this, but I'd prefer to add it with a decorator around this class such as UnreliableRedisCacheTagsChecksum or StaticCacheTagsChecksum register both existing (cached and non cached) into services and let the user choose among those two, I don't care about the one to use per default.

…ckend tests

…moved

pounard · 2015-04-19T19:33:27Z

src/Cache/RedisCacheTagsChecksum.php

@@ -24,7 +24,7 @@ class RedisCacheTagsChecksum implements CacheTagsChecksumInterface, CacheTagsInv
   *
   * @var array
   */
-  protected $tagCache = array();
+  protected $tagCache = [];


Sorry for the noise here, just found this to be more readable, even though I don't really care about writing it back as "array()".

pounard · 2015-04-19T19:39:54Z

src/Tests/Cache/ShardedPhpRedisUnitTest.php

+  /**
+   * Tests Drupal\Core\Cache\CacheBackendInterface::invalidateTags().
+   */
+  function testInvalidateTags() {


Since that my shared backend cannot guarantee by its algorithm that we can keep invalidated entries in all cases, I had to override the follow test cases. In all cases I at worst just removed a single line. The invalid entry keeping is an optional feature and not all backend will be able to do it as soon as you expect the backend itself to deal with expiration. I had to ugly patch my cache backend to force it to keep entries in some cases, and I'm not happy with this, because core test class forces this feature to be mandatory for all backends using it...

Makes sense, I'm OK with opening an issue for this, at the very least, the base class could have a $this->keepsInvalid or so property that a subclass can easily implement?

Yes, sure, this would be great!

pounard · 2015-04-19T20:57:21Z

Tests are failing on Travis but they pass on my box, I need feedback from your side.

Berdir · 2015-04-22T12:14:37Z

TODOLIST.txt

+     - If the same schema is used that the 7.x version, then there is no use
+       in sharding it, and should be stored along the router table replacement.
+
+ * Drupal\Core\Session\SessionHandler


This just got a lot easier with https://www.drupal.org/node/2228393, yes.

pounard · 2015-04-23T13:13:32Z

Not sure. I am using multi/pipeline for multiple cache get/set. But I'm not sure if that would make a difference that's big enough to be worth maintaining two separate implementations.

Pipelining does really bring a performance boost, but if it is the only one, it indeed might worth the shot to maintain only one implementation.

My last flush/deleteAll implementation keeps the value in a object property, to avoid repeated fetchs for it... I think that's a worthy optimization, otherwise we have to do an additional get for ever cache get?

I do agree about the flush timestamp static cache being a worthy optimization for most common uses cases, but it may cause trouble in concurrency scenarios, mostly during writes, that's why I didn't implement it this way. I still didn't reach a consensus with myself about that! In real life I got from time to time a few conflicts on feature revert and cache rebuilds in Drupal 7 sites, so I need to think deeper about it before taking any decision about that.

I have a separate method expandEntry() to convert the redis hash array into a cache object and check if it's still valid. I think that helps to keep get() shorter and easier to understand.
Similar for set(), I have a createEntryHash(). I'm totally fine with renaming them, but I think they are useful.

I have no strong opinion about the createEntryHash() and expandEntry() functions, I did wrote them originally in the Drupal 7 codebase to share the business code in get and set due to the fact I do maintain multiple implementations, but now the implementation is coupled with tags and harded to factorize, so I don't really see the point of keeping it. Nevertheless if it's something you care about, why not, I'll leave the task to factorize to you then :)

In get(), you immediately delete invalid entries. Is that really worth it? I think we can make the assumption that someone will actually rebuild and set that cache almost immediately anyway?

The fact that I delete directly entries in get() is because there is absolutely no other way to proceed, except by writing some kind of queue somewhere, thing that I definitely try to avoid to reduce the complexity. I don't want to rely upon garbage collection because there is no visibility over the frequency of when it happens and I don't want to end up with stalled or huge queues all over the place. The delete on read model is valid and working quite well, it allows to manage some additional space on the Redis side whenever we can (it's almost opportunistic) even if we know that in most cases Drupal will rewrite the key right after loading when invalidated, I kind of like it this way. We might change this behavior to optional in order to keep the invalid entries longer, but that would be the only benefit from this.

A Cache::validateTags($tags); call in set()

I should probably merge that in my implementation, it does warn by raising an exception at insert time when the caller does stupid thing, that's probably for the best, and it goes along the default backend behavior.

A small optimization to do multiple delete by passing an array of keys to get(), not sure if that is shardable?

Nope, in a sharded environement every key may arbitrarily set on an arbitrary server depending on your how your shard is configured and what is your hashing algorithm, it's definitely not possible to proceed to commands that work on multiple keys.

You usually have the real implementation in the single method, in my case it's in multiple, I don't really care which way round it is.

Yup me neither, there is no multiple implementation anymore in this backend, so it was just more natural to implement this way.

I implemented invalidateAll() using a cache-tag-per-bin, that then works with the allow invalid, but I'd be OK with dropping that.

Actually my implementation seems to work fine but it clears instead of invalidating, but since the backend can't guarantee that invalid entries are kept at this stade it actually does not change anything functionnaly, maybe this could be restored later by removing the delete on read algorithm.

So not that much different and a few of those might be useful to merge into your implementation?

I'd say indeed, we should merge, and maybe let the user be able to disable the delete on read behavior using settings configuration if they wish to, and that'd be perfect.

slashrsm · 2015-06-03T16:24:15Z

redis.services.yml

@@ -3,4 +3,4 @@ services:
    class: Drupal\redis\Cache\CacheBackendFactory
    arguments: ['@redis.factory', '@cache_tags.invalidator.checksum']
  redis.factory:
-    class: Drupal\redis\ClientFactory
+    class: Drupal\redis\ClientFactory


slashrsm · 2015-06-04T14:35:09Z

Not sure. I am using multi/pipeline for multiple cache get/set. But I'm not sure if that would make a difference that's big enough to be worth maintaining two separate implementations.

Pipelining does really bring a performance boost, but if it is the only one, it indeed might worth the shot to maintain only one implementation.

Just out of curiosity... do we have any idea about the extent of the performance boost we get with pipelining?

In get(), you immediately delete invalid entries. Is that really worth it? I think we can make the assumption that someone will actually rebuild and set that cache almost immediately anyway?

The fact that I delete directly entries in get() is because there is absolutely no other way to proceed, except by writing some kind of queue somewhere, thing that I definitely try to avoid to reduce the complexity. I don't want to rely upon garbage collection because there is no visibility over the frequency of when it happens and I don't want to end up with stalled or huge queues all over the place. The delete on read model is valid and working quite well, it allows to manage some additional space on the Redis side whenever we can (it's almost opportunistic) even if we know that in most cases Drupal will rewrite the key right after loading when invalidated, I kind of like it this way. We might change this behavior to optional in order to keep the invalid entries longer, but that would be the only benefit from this.

Could we set expiration time on invalidated items (http://redis.io/commands/expire) and achieve garbage collection this way? It seem this might work w/o much additional complexity, but I might be missing something.

So not that much different and a few of those might be useful to merge into your implementation?

I'd say indeed, we should merge, and maybe let the user be able to disable the delete on read behavior using settings configuration if they wish to, and that'd be perfect.

+1 on merging.

Berdir · 2015-07-13T21:41:59Z

I've created #7 so I can try to merge this. I had to make some changes and for example use REQUEST_TIME instead of time(), as that didn't work.. the entries that were meant to expire e.g. 3 seconds in the future were already expired. Unit tests are working fine now but the new web test that I added doesn't seem to pass for me, and it passes fine on 8.x-1.x. You're welcome to try your luck on that.

You can just merge it back into your PR if you can make anything work.

pounard · 2015-07-16T16:04:24Z

Just out of curiosity... do we have any idea about the extent of the performance boost we get with pipelining?

I didn't do any benachmarks, indeed.

Could we set expiration time on invalidated items (http://redis.io/commands/expire) and achieve garbage collection this way? It seem this might work w/o much additional complexity, but I might be missing something.

Actually it's already done, the whole point is to make all the entries volatile to avoid stalling keys. I did the explicit del calls at first glance because Drupal might sometime do thousands of get/set/del during cache wipe or cache rebuild, and it might worth the shot to clean this mess directly.

juanramonperez · 2016-02-05T16:59:50Z

src/Cache/CacheBase.php

@@ -158,7 +137,7 @@ protected function getExpiration($expire) {
    if ($expire == Cache::PERMANENT || $expire > $this->permTtl) {
      return $this->permTtl;
    }
-    return $expire - REQUEST_TIME;
+    return $expire - time();


Drupal recommends to use REQUEST_TIME instead time()

pounard reviewed Apr 18, 2015
View reviewed changes

pounard added 2 commits April 18, 2015 15:57

Added ShardablePhpRedis backend

d9fd625

Fixed tests of ShardedPhpRedis backend

b617f22

pounard reviewed Apr 19, 2015
View reviewed changes

pounard added 6 commits April 19, 2015 15:37

Added ShardedPhpRedis client implementation

81562c4

System may arbitrary choose the wrong implementation for the cache ba…

508f53e

…ckend tests

Fixed potential invalid item load as valid when a tag key is being re…

3f2e76e

…moved

Removed stupidly added invalidator service in services.yml file

646309d

Restored static cache into invalidator/checksum service

c33f9d1

The weirdest bug ever, I must have done a typo somewhere...

0fe256e

pounard reviewed Apr 19, 2015
View reviewed changes

Added todolist

b925998

pounard reviewed Apr 19, 2015
View reviewed changes

Berdir reviewed Apr 22, 2015
View reviewed changes

pounard added 3 commits April 23, 2015 13:45

RedisPrefixTrait::getNextIncrement() documentation

6d6f907

Added a few notes into the TODOLIST

ab4171c

Fixed a Drupal coding standard convention violation

38e2607

slashrsm reviewed Jun 3, 2015
View reviewed changes

Berdir mentioned this pull request Jul 11, 2015

ShardablePhpRedis backend #7

Open

juanramonperez reviewed Feb 5, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ShardablePhpRedis backend #5

Added ShardablePhpRedis backend #5

pounard commented Apr 18, 2015

pounard Apr 18, 2015

pounard Apr 19, 2015

pounard Apr 19, 2015

pounard Apr 19, 2015

Berdir Apr 22, 2015

pounard Apr 22, 2015

pounard commented Apr 19, 2015

Berdir Apr 22, 2015

pounard commented Apr 23, 2015

slashrsm Jun 3, 2015

slashrsm commented Jun 4, 2015

Berdir commented Jul 13, 2015

pounard commented Jul 16, 2015

juanramonperez Feb 5, 2016

Added ShardablePhpRedis backend #5

Are you sure you want to change the base?

Added ShardablePhpRedis backend #5

Conversation

pounard commented Apr 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pounard commented Apr 19, 2015

Choose a reason for hiding this comment

pounard commented Apr 23, 2015

Choose a reason for hiding this comment

slashrsm commented Jun 4, 2015

Berdir commented Jul 13, 2015

pounard commented Jul 16, 2015

Choose a reason for hiding this comment