[Cache] Memcached and Memcache Adapters #20752

robfrawley · 2016-12-05T00:49:46Z

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

Provides cache adapters for memcached and memcache extensions. Included unit tests cover entire code addition. Based heavily on existing Redis tests.

robfrawley · 2016-12-05T01:24:53Z

Memcached extension on PHP 5.5 is broken when attempting to request the last result code: https://travis-ci.org/symfony/symfony/jobs/181224208#L2409

Anyone know if this is a current bug in that combination of the extension and PHP or if, perhaps, Tavis simply has an older, outdated extension running that has already been fixed in more-recent extension builds against that PHP API version?

nicolas-grekas · 2016-12-05T12:34:44Z

See #20743 for another PR on the same topic.

nicolas-grekas

Thanks for the PR!
Looks good, but on a few points:

IMHO, there are to many public helpers. RedisAdapter has only one helper to create a connection from a DSN. I think that if we can do the same, it would be better.
configuring the connection should not be the responsibility of this class (setOption, addServer, etc.) so I'd remove these.
the values are double-serialized. Shouldn't we let memcached handle (un)serialization itself?

nicolas-grekas · 2016-12-05T12:56:58Z

src/Symfony/Component/Cache/Adapter/MemcachedAdapter.php

+        foreach ($this->memcached->getMulti($ids) as $id => $val) {
+            try {
+                yield $id => parent::unserialize($val);
+            } catch (\Exception $e) {


What is this try/catch for? I don't think we need it.

Left over from Redis basis for this class. Gone at this point. Also, re: serializtion: I assumed there was a specific requirement to handle serialization using the AbstractAdapter implementation (I guess Redis doesn't handle this for you?). Anyway, the double serialization has been removed at this point. Will push the latest code changes shortly.

Try/Catch removed. Double serialization removed.

nicolas-grekas · 2016-12-05T13:04:21Z

src/Symfony/Component/Cache/Adapter/MemcachedAdapter.php

+     */
+    protected function doDelete(array $ids)
+    {
+        $deletions = array_filter((array) $this->memcached->deleteMulti($ids), function ($ret) {


Could a foreach based implem be more readable? This $deletions array looks superfluous to me. What about:

$toDelete = count($ids); foreach ($this->memcached->deleteMulti($ids) as $result) { if (true === $result || \Memcached::RES_NOTFOUND === $result) { --$toDelete; } } return 0 === $toDelete;

That does look cleaner; I'm with you on that --- will make such a change.

Implemented.

nicolas-grekas · 2016-12-05T13:27:39Z

src/Symfony/Component/Cache/Adapter/MemcachedAdapter.php

+     */
+    protected function doClear($namespace)
+    {
+        return $this->memcached->flush()


If possible, it would be better to delete only the keys that start with $namespace.
Looking at https://gist.github.com/stetic/2929166 (Ruby) or http://stackoverflow.com/a/34125587 (memcached ext.) there seem to be a way to do so properly.
Could you please try how we could to something similar with the memcached ext.?

Yeah, so I've been looking into this since I wrote this initial adapter a few weeks back. The issue revolves around the getAllKeys() method, which would easily allow us to iterate over the keys and decide what to delete......except that method returns false instead of working properly on 80% of the configurations I've tested on. Apparently you need very specific cercumstances to come together for it to work properly. Should I implement that and then fallback to the global flush?

Also, the article you linked to is the memcache extension (notice it is missing the d --- its a different extension). This extension, memcached, doesn't offer the same ability. But, I could always try to use that method if the user also has the memcache extension installed?

Thoughts?

Perhaps something like this for those who happen to have a working combination of the extension/system-library/php-api:

if (false === $ids = $this->client->getAllKeys()) { return $this->client->flush() && $this->client->getResultCode() === \Memcached::RES_SUCCESS; } return $this->doDelete(array_filter($ids, function ($id) use ($namespace) { return false !== strpos($id, $namespace); }));

Looks like this issue is related to php-memcached-dev/php-memcached#203
Let's go with flushing then when it fails.
What about doint it this way:

if (!isset($namespace[0]) || false === $ids = $this->client->getAllKeys()) { return $this->client->flush(); } while (true) { $ids = array_filter($ids, function ($id) use ($namespace) { return 0 === strpos($id, $namespace); }); if ($ids) { $this->doDelete($ids); } if (!$ids || !$ids = $this->client->getAllKeys()) { return true; } }

@nicolas-grekas What up with the !isset($namespace[0])? Can the namespace passed in here be an array?

This code also works with a string, null or false. That's the code in use in other places in the component for this check.

robfrawley · 2016-12-05T15:55:21Z

As for #20743, that takes on way too much responsibility, IMHO. I prefer extending the existing (and well tested) AbstractAdapter. Just my two cents.

stof · 2016-12-05T16:00:36Z

src/Symfony/Component/Cache/Adapter/MemcachedAdapter.php

+{
+    const DEFAULT_HOST = '127.0.0.1';
+    const DEFAULT_PORT = 11211;
+    const DEFAULT_WEIGHT = -1;


IMO, these should not be constants (or they should be internal constants at least)

@stof You mean something more like how the Redis adapter handles defaults? https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Cache/Adapter/RedisAdapter.php#L25

Defaults connection details moved to private class array property to mirror Redis implementation.

stof · 2016-12-05T16:02:48Z

I'm with @nicolas-grekas here. We should not have all these helper methods about configuring Memcached itself

robfrawley · 2016-12-05T16:16:15Z

@stof I already started some work on this per @nicolas-grekas comment; I have refactored down to the following so far:

public static function createAdapter(array $servers = array(), array $options = array(), $persistentId = null);
public function setupAdapter(array $servers = array(), array $options = array());
public function getClient();

The following prior public methods have been made private addServer() and setOption() as they are still used by the setupAdapter method. Is this more in-line with what you guys would be looking for? Or is that still too much in terms of a public API?

nicolas-grekas · 2016-12-05T16:24:04Z

I guess you didn't push yet :)
IMHO, we should only keep createClient (getClient and createAdapter should be removed.)
As for RedisAdapter, createClient should take a DSN string as first argument (and a $options array as second). The benefit in doing so is that the memcached connection is then easily configurable with a simple environment variable.

nicolas-grekas · 2016-12-05T16:25:15Z

Note that if this is too much work for the current PR, we can very well split it in two and merge the pure Adapter code first.

robfrawley · 2016-12-05T16:36:27Z

@nicolas-grekas @stof Initial changes pushed; likely some more to come though, just wanted to let you guys see where it's at so far... Let me know if this is on the right track. Thanks for the reviews and comments guys!

robfrawley · 2016-12-05T16:48:57Z

@nicolas-grekas I don't know if I agree with moving toward a simple DSN for this adapter. The benefits of using Memcached over some of the alternatives is the ability to passing an unlimited number of servers of various weights, to easily support fall-over, etc...

robfrawley · 2016-12-05T16:58:17Z

@nicolas-grekas Alternately, if we changed createAdapter to allow a simple DSN, but retained the signature of setupAdapter, that may be a solid middle ground in terms of allowing simple usage by-default, while retaining the ability for advanced setups too. For example, changing the signatures to:

public static function createAdapter($dsn = null, array $options = array(), $persistentId = null);
public function setupAdapter(array $servers = array(), array $options = array());

Would that possibly allow for a solid middle-ground between ease of use similarity between other adapter, while still allowing for more advanced configuration easily enough by simply adding the following to ones service definition.

calls:
    - [ setup, [ { "%memcached_dsn_1%", "%memcached_dsn_2%" }, { "OPT_DISTRIBUTION": "DISTRIBUTION_CONSISTENT", "OPT_LIBKETAMA_COMPATIBLE": true } ] ]

robfrawley · 2016-12-05T17:01:55Z

May be related to PHP 5.5 Travis test error: https://bugs.php.net/bug.php?id=59679

stof · 2016-12-05T17:09:49Z

I don't know if I agree with moving toward a simple DSN for this adapter. The benefits of using Memcached over some of the alternatives is the ability to passing an unlimited number of servers of various weights, to easily support fall-over, etc...

@robfrawley if you want to configure the Memcached client with more power, this is still possible: create the Memcached instance yourselves and pass it to the constructor.

nicolas-grekas · 2016-12-05T17:13:59Z

Note also that it's perfectly possible to have a DSN that could configures several servers.
See e.g. https://github.com/phpredis/phpredis/blob/feature/redis_cluster/cluster.markdown#readme telling ini settings with multi server configs.

robfrawley · 2016-12-05T18:42:30Z

@nicolas-grekas and @stof Initial implementation using DSNs instead of the prior array approach. The memcache "weight" option is can be passed using a query in the DSN (?weight=<int>). Some examples:

memcached://127.0.0.1:11211?weight=10
memcached://127.0.0.1:11212?weight=40
memcached://127.0.0.1:11213?weight=50

The current implementation retains the ability to pass a complex config, as well as offers a user-friendly experience for those looking for a basic, single server instance of this adapter.

robfrawley · 2016-12-06T09:15:40Z

Pushed a second commit (that we can rollback or push into another PR if desired), that adds an adapter for the Memcache extension (in addition to the original Memcached adapter) and pulls out all the shared logic into an abstract class. Thoughts?

nicolas-grekas · 2016-12-05T20:09:32Z

src/Symfony/Component/Cache/Adapter/MemcachedAdapter.php

+    {
+        $toDelete = count($ids);
+
+        foreach ((array) $this->client->deleteMulti($ids) as $result) {


Why casting to array? Can it return something else? The doc doesn't say so.

Older versions of the extension return a simple bool (versus the current behavior of returning bool[]) for "all succeeded" or "one or more failures". The cast is there to allow fallback support for older versions of the extension. Without this we'd get a PHP warning (I think?) for incorrect data type passed into the foreach.

Do you have a link that describes this behavior?

nicolas-grekas · 2016-12-06T11:45:24Z

src/Symfony/Component/Cache/Adapter/AbstractMemcacheAdapter.php

+/**
+ * @author Rob Frawley 2nd <rmf@src.run>
+ */
+abstract class AbstractMemcacheAdapter extends AbstractAdapter


this should be turned into a trait with the @internal annotation (see other traits in the Adapter folder)

nicolas-grekas · 2016-12-06T11:46:03Z

src/Symfony/Component/Cache/Adapter/MemcacheAdapter.php

+        return 0 === count($remaining);
+    }
+
+    protected function getAllIdsMatching($namespace)


can be private when a trait is used instead of abstract (same in the other class)

nicolas-grekas · 2016-12-06T11:47:10Z

src/Symfony/Component/Cache/Adapter/MemcacheAdapter.php

+                if (!is_array($slabMetadata)) {
+                    continue;
+                }
+                foreach ($this->client->getExtendedStats('cachedump', (int) $slabId, 0) as $slabIds) {


1000 instead of 0 for the limit? (you already handle looping in doClear)

No preference on this from me. Changed to 1000. Seems find to handle it that way.

nicolas-grekas · 2016-12-06T11:47:53Z

src/Symfony/Component/Cache/Adapter/MemcacheAdapter.php

+            }
+        }
+
+        return array_filter((array) $ids, function ($id) use ($namespace) {


no need for this $ids array, its consuming memory for no reason

I don't follow. How would we remove the need for this array?

nicolas-grekas · 2016-12-06T11:49:05Z

src/Symfony/Component/Cache/Adapter/MemcacheAdapter.php

+        });
+    }
+
+    protected function addServer($dsn)


all protected method should be private in fact (we are usd to think twice before accepting any public or protected method because they are covered by our BC promise)

nicolas-grekas · 2016-12-06T11:49:31Z

src/Symfony/Component/Cache/Adapter/MemcachedAdapter.php

+    protected function doDelete(array $ids)
+    {
+        $toDelete = count($ids);
+        foreach ((array) $this->client->deleteMulti($ids) as $result) {


array cast really needed?

Older versions of the extension return a bool instead of a bool[], so yes, the case is needed.

nicolas-grekas · 2016-12-06T11:49:55Z

src/Symfony/Component/Cache/Adapter/MemcachedAdapter.php

+        return $success && $this->isPreviousClientActionSuccessful();
+    }
+
+    protected function getAllIdsMatching($namespace)


getIdsByPrefix?

nicolas-grekas · 2016-12-06T11:51:04Z

src/Symfony/Component/Cache/Adapter/MemcachedAdapter.php

+        return $this->client->getResultCode() === \Memcached::RES_SUCCESS;
+    }
+
+    protected function isServerInClientPool($host, $port)


I don't think it's the responsibility of this class to handle this. Let's assume the user knows what it does.

This is majorly important depending on the environment PHP is running in. For example, given the following service definition

my.cache: class: Symfony\Component\Cache\Adapter\MemcachedAdapter calls: [setup, [["%memcached_dsn_01%", "%memcached_dsn_02%"]]

On every page request, the registered servers are actually persisted (again, depending on environment), meaning after 100 page requests you'll have 200 servers registered with the extension (all of which are duplicates of the first two).

Hence the requirement, and importance, for this check.

robfrawley · 2016-12-09T16:04:10Z

Latest changes per comments from nicolas-grekas --- also, refactored tests.

@nicolas-grekas and @stof Apologies on the inclusion of @ mentions in my commit message. I didn't realize the implications of that on each new push with regard to spamming you guys with messages. This has been fixed in the latest code push. Appreciate your patience and understanding on that, as it was pointed out a few days ago as well, I simply didn't follow what @nicolas-grekas was trying to convey.

robfrawley · 2016-12-09T16:50:39Z

src/Symfony/Component/Cache/Adapter/MemcacheAdapter.php

+     */
+    public static function create($dsn = null)
+    {
+        if (!extension_loaded('memcache') || !version_compare(phpversion('memcache'), '3.0.8', '>')) {


The behavior of this extension on version 3.0.8 seems completely different from the behavior of 3.0.9 - IDK why that would be the case with what should be a patch release, but tests fail with any version below 3.0.9.

robfrawley · 2016-12-09T19:22:43Z

Also, any ideas as to what to do about the broken extension version Travis is running for Memcached on its PHP 5.6 containers? https://travis-ci.org/symfony/symfony/jobs/182643135#L2670

nicolas-grekas · 2016-12-10T08:23:01Z

I spent some time reading this and the doc for both extensions. Given the poor feature set of the memcache ext, should we support it at all? I'd prefer not.

I think the dsn/connection configuration code needs a separate PR: I've to much comments on it right now. I'd prefer focusing on the cache pool part first.

Since connections are lazily created, we should be better at error handling + logging. That's something only the Memcached ext allows, this a big reason why I prefer it.

robfrawley · 2016-12-10T16:04:12Z

@nicolas-grekas See #20858

robfrawley · 2016-12-10T21:38:24Z

Replaced by #20863 and #20858.

This PR was merged into the 3.3-dev branch. Discussion ---------- [Cache] Simple Memcached Adapter | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | | Related PRs | #20863, ~~#20752~~ Commits ------- 12de2ae memcached cache pool adapter

carsonbot added Status: Needs Review Cache Feature labels Dec 5, 2016

robfrawley changed the title ~~[Cache]~~ [Cache] Simple Memcached Adapter Implementation Dec 5, 2016

robfrawley force-pushed the feature-cache-adapter-memcached branch 2 times, most recently from 51b8200 to 5b21e75 Compare December 5, 2016 01:15

robfrawley force-pushed the feature-cache-adapter-memcached branch from 5b21e75 to 6b637a7 Compare December 5, 2016 01:30

nicolas-grekas mentioned this pull request Dec 5, 2016

[Cache] Add MemcachedAdapter #20743

Closed

nicolas-grekas reviewed Dec 5, 2016

View reviewed changes

stof reviewed Dec 5, 2016

View reviewed changes

robfrawley force-pushed the feature-cache-adapter-memcached branch 3 times, most recently from 65e21a1 to 2336e57 Compare December 5, 2016 16:42

robfrawley force-pushed the feature-cache-adapter-memcached branch from 2336e57 to a287fb8 Compare December 5, 2016 18:40

robfrawley force-pushed the feature-cache-adapter-memcached branch 2 times, most recently from 7cd7dd8 to 1fb3338 Compare December 5, 2016 19:21

robfrawley force-pushed the feature-cache-adapter-memcached branch 6 times, most recently from 9e5f7a5 to 066c72b Compare December 6, 2016 09:14

robfrawley force-pushed the feature-cache-adapter-memcached branch from 066c72b to a8215e5 Compare December 6, 2016 09:18

nicolas-grekas reviewed Dec 6, 2016

View reviewed changes

nicolas-grekas modified the milestones: 3.3, 3.x Dec 6, 2016

robfrawley force-pushed the feature-cache-adapter-memcached branch from 54f546f to 0b7f4ef Compare December 9, 2016 16:02

robfrawley force-pushed the feature-cache-adapter-memcached branch 3 times, most recently from b9c937d to 73d5ef7 Compare December 9, 2016 16:45

robfrawley commented Dec 9, 2016

View reviewed changes

robfrawley changed the title ~~[Cache] Simple Memcached Adapter Implementation~~ [Cache] Memcached and Memcache Adapter Implementations Dec 9, 2016

robfrawley changed the title ~~[Cache] Memcached and Memcache Adapter Implementations~~ [Cache] Memcached and Memcache Adapters Dec 9, 2016

robfrawley mentioned this pull request Dec 10, 2016

[Cache] Simple Memcached Adapter #20858

Merged

cache adapters for memcached and memcache extensions

dec61d0

robfrawley force-pushed the feature-cache-adapter-memcached branch from 73d5ef7 to dec61d0 Compare December 10, 2016 18:12

robfrawley mentioned this pull request Dec 10, 2016

[Cache] Advanced Memcached Adapter #20863

Closed

robfrawley closed this Dec 10, 2016

[Cache] Memcached and Memcache Adapters #20752

[Cache] Memcached and Memcache Adapters #20752

Conversation

robfrawley commented Dec 5, 2016 • edited Loading

robfrawley commented Dec 5, 2016 • edited Loading

nicolas-grekas commented Dec 5, 2016

nicolas-grekas left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

robfrawley Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Dec 5, 2016

robfrawley commented Dec 5, 2016 • edited Loading

nicolas-grekas commented Dec 5, 2016

nicolas-grekas commented Dec 5, 2016

robfrawley commented Dec 5, 2016 • edited Loading

robfrawley commented Dec 5, 2016

robfrawley commented Dec 5, 2016 • edited Loading

robfrawley commented Dec 5, 2016 • edited Loading

stof commented Dec 5, 2016

nicolas-grekas commented Dec 5, 2016

robfrawley commented Dec 5, 2016 • edited Loading

robfrawley commented Dec 6, 2016

Choose a reason for hiding this comment

robfrawley Dec 9, 2016 • edited Loading

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

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

robfrawley Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

robfrawley commented Dec 9, 2016 • edited Loading

robfrawley Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

robfrawley commented Dec 9, 2016 • edited Loading

nicolas-grekas commented Dec 10, 2016 • edited Loading

robfrawley commented Dec 10, 2016

robfrawley commented Dec 10, 2016

robfrawley commented Dec 5, 2016 •

edited

Loading

robfrawley commented Dec 5, 2016 •

edited

Loading

robfrawley Dec 5, 2016 •

edited

Loading

robfrawley Dec 5, 2016 •

edited

Loading

robfrawley Dec 5, 2016 •

edited

Loading

robfrawley commented Dec 5, 2016 •

edited

Loading

robfrawley commented Dec 5, 2016 •

edited

Loading

robfrawley commented Dec 5, 2016 •

edited

Loading

robfrawley commented Dec 5, 2016 •

edited

Loading

robfrawley commented Dec 5, 2016 •

edited

Loading

robfrawley Dec 9, 2016 •

edited

Loading

robfrawley Dec 9, 2016 •

edited

Loading

robfrawley Dec 9, 2016 •

edited

Loading

robfrawley commented Dec 9, 2016 •

edited

Loading

robfrawley Dec 9, 2016 •

edited

Loading

robfrawley commented Dec 9, 2016 •

edited

Loading

nicolas-grekas commented Dec 10, 2016 •

edited

Loading