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

Added ShardablePhpRedis backend #5

Open
wants to merge 12 commits into
base: 8.x-1.x
Choose a base branch
from
3 changes: 3 additions & 0 deletions redis.services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

29 changes: 15 additions & 14 deletions src/Cache/CacheBase.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@
*/
abstract class CacheBase implements CacheBackendInterface {

use RedisPrefixTrait;
use RedisPrefixTrait {
getKey as getParentKey;
}

/**
* Temporary cache items lifetime is infinite.
Expand Down Expand Up @@ -132,18 +134,6 @@ public function invalidate($cid) {
$this->invalidateMultiple([$cid]);
}

/**
* Return the key for the given cache key.
*/
public function getKey($cid = NULL) {
if (NULL === $cid) {
return $this->getPrefix() . ':' . $this->bin;
}
else {
return $this->getPrefix() . ':' . $this->bin . ':' . $cid;
}
}

/**
* Calculate the correct expiration time.
*
Expand All @@ -158,7 +148,7 @@ protected function getExpiration($expire) {
if ($expire == Cache::PERMANENT || $expire > $this->permTtl) {
return $this->permTtl;
}
return $expire - REQUEST_TIME;
return $expire - time();

Choose a reason for hiding this comment

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

Drupal recommends to use REQUEST_TIME instead time()

}

/**
Expand Down Expand Up @@ -205,4 +195,15 @@ public function setPermTtl($ttl = NULL) {
}
}

/**
* {@inheritdoc}
*/
public function getKey($parts) {
if (is_string($parts)) {
$parts = [$parts];
}
array_unshift($parts, $this->bin);
return $this->getParentKey($parts);
}

}
4 changes: 2 additions & 2 deletions src/Cache/PhpRedis.php
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array

// Build the cache item and save it as a hash array.
$entry = $this->createEntryHash($cid, $data, $expire, $tags);
$pipe = $this->client->multi(\REdis::PIPELINE);
$pipe = $this->client->multi(\Redis::PIPELINE);
$pipe->hMset($key, $entry);
$pipe->expire($key, $ttl);
$pipe->exec();
Expand Down Expand Up @@ -240,7 +240,7 @@ protected function expandEntry(array $values, $allow_invalid) {
// Check expire time, allow to have a cache invalidated explicitly, don't
// check if already invalid.
if ($cache->valid) {
$cache->valid = $cache->expire == Cache::PERMANENT || $cache->expire >= REQUEST_TIME;
$cache->valid = $cache->expire == Cache::PERMANENT || $cache->expire >= time();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to my fixup in generic cache unit tests in core, this usage of the REQUEST_TIME constant then caused invalid results in the get() method.


// Check if invalidateTags() has been called with any of the items's tags.
if ($cache->valid && !$this->checksumProvider->isValid($cache->checksum, $cache->tags)) {
Expand Down
70 changes: 33 additions & 37 deletions src/Cache/RedisCacheTagsChecksum.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,44 +51,42 @@ function __construct(ClientFactory $factory) {
* {@inheritdoc}
*/
public function invalidateTags(array $tags) {
$keys_to_increment = [];
foreach ($tags as $tag) {
// Only invalidate tags once per request unless they are written again.
if (isset($this->invalidatedTags[$tag])) {
continue;
}
$this->invalidatedTags[$tag] = TRUE;
unset($this->tagCache[$tag]);
$keys_to_increment[] = $this->getTagKey($tag);
}
if ($keys_to_increment) {
$multi = $this->client->multi(\Redis::PIPELINE);
foreach ($keys_to_increment as $key) {
$multi->incr($key);
}
$multi->exec();
$tagKey = $this->getKey(['tag', $tag]);
$current = $this->client->get($tagKey);
$this->client->set($tagKey, $this->getNextIncrement($current));
}
}

/**
* {@inheritdoc}
*/
public function getCurrentChecksum(array $tags) {
/*
* @todo Restore cache
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As said here, this is still a proof of concept, but it do work nicely. The cache may be restored in order to gain some round trips to Redis, I'll do this, I am thought making the pull request as-is to have your feedback faster.

*
// Remove tags that were already invalidated during this request from the
// static caches so that another invalidation can occur later in the same
// request. Without that, written cache items would not be invalidated
// correctly.
foreach ($tags as $tag) {
unset($this->invalidatedTags[$tag]);
}
*/
return $this->calculateChecksum($tags);
}

/**
* {@inheritdoc}
*/
public function isValid($checksum, array $tags) {
return $checksum == $this->calculateChecksum($tags);
foreach ($tags as $tag) {
$current = $this->client->get($this->getKey(['tag', $tag]));
if ($checksum < $current) {
return FALSE;
}
}
return TRUE;
}

/**
Expand All @@ -97,16 +95,27 @@ public function isValid($checksum, array $tags) {
public function calculateChecksum(array $tags) {
$checksum = 0;

$fetch = array_values(array_diff($tags, array_keys($this->tagCache)));
if ($fetch) {
$keys = array_map(array($this, 'getTagKey'), $fetch);
foreach ($this->client->mget($keys) as $index => $invalidations) {
$this->tagCache[$fetch[$index]] = $invalidations ?: 0;
foreach ($tags as $tag) {

$current = $this->client->get($this->getKey(['tag', $tag]));

if (!$current) {
// Tag has never been created yet, so ensure it has an entry in Redis
// database. When dealing in a sharded environment, the tag checksum
// itself might have been dropped silently, case in which giving back
// a 0 value can cause invalided cache entries to be considered as
// valid back.
// Note that doing that, in case a tag key was dropped by the holding
// Redis server, all items based upon the droppped tag will then become
// 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}

foreach ($tags as $tag) {
$checksum += $this->tagCache[$tag];
if ($checksum < $current) {
$checksum = $current;
}
}

return $checksum;
Expand All @@ -120,17 +129,4 @@ public function reset() {
$this->invalidatedTags = array();
}

/**
* Return the key for the given cache tag.
*
* @param string $tag
* The cache tag.
*
* @return string
* The prefixed cache tag.
*/
protected function getTagKey($tag) {
return $this->getPrefix() . ':cachetags:' . $tag;
}

}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really no need of a function to get rid of a single line of code, it's being used only twice in the whole code, let's await for the third use case to popup before trying to factorize this. Premature factorization comes at the price of less readible, less maintainable, and more circonvoluted code.

Loading