[Cache] Add DSN, createClient & better error reporting to MemcachedAdapter #21108

nicolas-grekas · 2016-12-30T14:43:49Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	symfony/symfony-docs#7265

Replaces #20863
ping @robfrawley: would you mind opening a doc PR for this?

robfrawley · 2016-12-30T16:59:35Z

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

+        }
+        if ($client->getOption(\Memcached::OPT_NO_BLOCK)) {
+            throw new CacheException('MemcachedAdapter: OPT_NO_BLOCK must be disabled.');
+        }


I understand the serializer requirement, but why are we forcing binary protocol and no_block=false?

Binary because we must support all chars in keys especially spaces, per psr-6.
No block because we need guarantees when reading/writing to the cache. Dealing with async would make the code complex for no benefit.

Regarding binary_protocol requirement: I'm on board with this.

But, regarding no_block requirement: I disagree. The intended use of this option doesn't require any additional code, and, in fact, writing such code would invalidate the entire intended usage and benefits of no_block. The expectation when it is enabled is that the user doesn't need guarantees when writing. It is a commonly used option and generally doesn't change expectations; due to the design of memcached, one can never assume a value exists as it may have been purged at any time by the LRU algorithm, and will definitely not exist after a restart of the daemon.

fixed for no_block

robfrawley · 2016-12-30T18:37:32Z

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

+            unset($options['persistent_id'], $options['username'], $options['password']);
+
+            // set client's options
+            foreach ($options as $name => $value) {


This needs to be foreach (array_reverse($options) as $name => $value) as seen in my PR, otherwise defaul values that affect multiple options (such as libketama_compatible) will overwrite use-provided values.

I think reverse or not, it's the responsibility of the user to order correctly. I'd better behave the same as the native setOptions so that expectations don't have to be reversed when using our lib...

It's due to our code, though. The default option of libketama_compatible in combination with $options += static::$defaultClientOptions; means a user cannot change, for example, the hash algorithm, ever...as our default of libketama_compatible will be applied after the user-provided option (for example ['hash' => 'something-else']), resetting the hash to md5. The options need to be applied in reverse order to allow the user to overwrite our defaults.

To clarify: the user should, rightfully, assume that any options they provide will overwrite our defaults. The issue is, some options actually affect multiple options. Without reversing the array, the user options will not always result in a proper assignment; while reversing the options ensures user options always overwrite defaults.

It's not the order of options that is important, it's the order of default options to user options that is important. Default options must be applied first, so either the assignment needs to change:

# from this: $options += static::$defaultClientOptions; # to this (or something similar): foreach (static::$defaultClientOptions as $name => $option) { if (!array_key_exists($name, $options) { array_unshift($options, [$name => $option]); } }

OR the order of options needs to be reversed:

# from this: foreach ($options as $name => $value) {} # to this (or something similar): foreach (array_reverse($options) as $name => $value) {}

Otherwise options will behave unexpectedly for the user.

understood, fixed also

robfrawley · 2016-12-30T18:49:04Z

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

+            }
+
+            // set client's servers, taking care of persistent connections
+            if (!$client->isPristine()) {


We shouldn't add servers after a connection is open (even when the user has added an additional server to their configuration)?

Not sure to get what you mean. Do you see anything wrong with this logic?
Here it is: if the connection is not pristine (meaning it's a 2nd use persistent connection), then we check if there is any change in the list of server. If there is, we reset the list and set the new one.

Ok. Had trouble following the code, but makes sense now.

robfrawley · 2016-12-30T19:24:13Z

A documentation PR for #20863 is already open (see issue symfony/symfony-docs#7256 and PR symfony/symfony-docs#7265) and it doesn't require much at all to update it for this PR. I will ensure it is accurate after the implementation details I've asked about are handled/clarified. Thanks!

robfrawley · 2016-12-30T23:24:19Z

I'd add a test to cover the serialize requirement:

    /**
     * @expectedException \Symfony\Component\Cache\Exception\CacheException
     * @expectedExceptionMessage MemcachedAdapter: PHP or IGBINARY must be used.
     */
    public function testOptionSerializer()
    {
        new MemcachedAdapter(MemcachedAdapter::createConnection(array(), array('serializer' => 'json')));
    }

nicolas-grekas · 2016-12-31T09:19:19Z

@robfrawley test case added, thanks. Note also that considering your comment about no block, and the use case here (a cache layer), I enabled no-block by default. Tests are green which means it's good for PSR-6!

robfrawley · 2016-12-31T17:38:15Z

Platform.sh is not properly building the documentation for a bunch of PRs ATM, but I think the docs are accurate with regard to this PR (updated to show correct defaults per your latest updates): give it a look here.

xabbuh · 2016-12-31T18:58:09Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CachePoolPass.php

            $dsn = $name;

            if (!$container->hasDefinition($name = md5($dsn))) {
-                $definition = new Definition(\Redis::class);
+                $definition = new Definition('_');


What about using AdapterInterface::class to not make it harder for bundles that do static analyses of the container?

xabbuh · 2016-12-31T18:59:54Z

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

+            return MemcachedAdapter::createConnection($dsn, $options);
+        }
+
+        throw new InvalidArgumentException(sprintf('Unsupported DSN: %s.', $dsn));


Should the AbstractAdapter really be aware of all adapters that require a connection?

This is a stateless static method, no need for yet another class. This has to be somewhere :)
Other comments addressed.

xabbuh · 2016-12-31T19:01:16Z

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

+        }
+        $opt = $client->getOption(\Memcached::OPT_SERIALIZER);
+        if (\Memcached::SERIALIZER_PHP !== $opt && \Memcached::SERIALIZER_IGBINARY !== $opt) {
+            throw new CacheException('MemcachedAdapter: serializer must be PHP or IGBINARY.');


Should we use OPT_SERIALIZER instead of serializer here to be consistent with the exception message below?

The way this code is setup and documented, you pass serializer, not opt_serializer, so I would argue the inverse: the other exception message should be amend to:

binary_protocol must be used

xabbuh · 2016-12-31T19:02:26Z

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

+        if (is_string($servers)) {
+            $servers = array($servers);
+        } elseif (!is_array($servers)) {
+            throw new InvalidArgumentException(sprintf('MemcachedAdapter::createClient() expects array or string as first argument, %s given.', get_type($servers)));


gettype()

…apter

xabbuh · 2017-01-03T16:58:23Z

👍

robfrawley · 2017-01-03T19:59:08Z

@nicolas-grekas Do we want the RedisAdapter to accept an array of DSNs like the MemcacheAdapter; I'm looking into bringing the RedisAdapter documentation in-line with the new MemcacheAdapter documentation, re: symfony/symfony-docs/#7265@269875795. It appears it does not at this time.

nicolas-grekas · 2017-01-04T06:51:27Z

@robfrawley it would be great yes

nicolas-grekas · 2017-01-04T06:52:02Z

Thank you @robfrawley.

@robfrawley

… to MemcachedAdapter (nicolas-grekas, robfrawley) This PR was merged into the 3.3-dev branch. Discussion ---------- [Cache] Add DSN, createClient & better error reporting to MemcachedAdapter | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#7265 Replaces #20863 ping @robfrawley: would you mind opening a doc PR for this? Commits ------- 87030b4 [cache] Add tests for MemcachedAdapter::createClient() e109438 [Cache] Add DSN, createClient & better error reporting to MemcachedAdapter

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

nicolas-grekas mentioned this pull request Dec 30, 2016

[Cache] Advanced Memcached Adapter #20863

Closed

nicolas-grekas force-pushed the feature-advanced-memcached-cache-pool branch 9 times, most recently from 70e509a to 6d35624 Compare December 30, 2016 15:43

nicolas-grekas added this to the 3.3 milestone Dec 30, 2016

nicolas-grekas force-pushed the feature-advanced-memcached-cache-pool branch 2 times, most recently from 95ad534 to 6e68d2c Compare December 30, 2016 16:26

robfrawley reviewed Dec 30, 2016

View reviewed changes

nicolas-grekas force-pushed the feature-advanced-memcached-cache-pool branch 3 times, most recently from d2565af to 9e899a4 Compare December 30, 2016 21:37

nicolas-grekas force-pushed the feature-advanced-memcached-cache-pool branch from 9e899a4 to 4e31fea Compare December 31, 2016 09:15

nicolas-grekas force-pushed the feature-advanced-memcached-cache-pool branch from 4e31fea to 95c9777 Compare December 31, 2016 09:29

xabbuh reviewed Dec 31, 2016

View reviewed changes

[Cache] Add DSN, createClient & better error reporting to MemcachedAd…

e109438

…apter

nicolas-grekas force-pushed the feature-advanced-memcached-cache-pool branch 3 times, most recently from 36fd01e to 3eae606 Compare January 2, 2017 17:04

[cache] Add tests for MemcachedAdapter::createClient()

87030b4

nicolas-grekas force-pushed the feature-advanced-memcached-cache-pool branch from 3eae606 to 87030b4 Compare January 3, 2017 16:10

nicolas-grekas merged commit 87030b4 into symfony:master Jan 4, 2017

nicolas-grekas deleted the feature-advanced-memcached-cache-pool branch January 4, 2017 06:52

xabbuh mentioned this pull request Apr 20, 2017

[Cache] Documentation for default_memcached_provider symfony/symfony-docs#7828

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cache] Add DSN, createClient & better error reporting to MemcachedAdapter #21108

[Cache] Add DSN, createClient & better error reporting to MemcachedAdapter #21108

nicolas-grekas commented Dec 30, 2016 •

edited

Loading

robfrawley Dec 30, 2016

nicolas-grekas Dec 30, 2016

robfrawley Dec 30, 2016 •

edited

Loading

nicolas-grekas Dec 30, 2016

robfrawley Dec 30, 2016 •

edited

Loading

nicolas-grekas Dec 30, 2016

robfrawley Dec 30, 2016 •

edited

Loading

robfrawley Dec 30, 2016 •

edited

Loading

nicolas-grekas Dec 30, 2016

robfrawley Dec 30, 2016 •

edited

Loading

nicolas-grekas Dec 30, 2016 •

edited

Loading

robfrawley Dec 31, 2016

robfrawley commented Dec 30, 2016 •

edited

Loading

robfrawley commented Dec 30, 2016

nicolas-grekas commented Dec 31, 2016

robfrawley commented Dec 31, 2016 •

edited

Loading

xabbuh Dec 31, 2016

xabbuh Dec 31, 2016

nicolas-grekas Jan 2, 2017

xabbuh Dec 31, 2016

robfrawley Dec 31, 2016 •

edited

Loading

xabbuh Dec 31, 2016

xabbuh commented Jan 3, 2017

robfrawley commented Jan 3, 2017 •

edited

Loading

nicolas-grekas commented Jan 4, 2017

nicolas-grekas commented Jan 4, 2017

[Cache] Add DSN, createClient & better error reporting to MemcachedAdapter #21108

[Cache] Add DSN, createClient & better error reporting to MemcachedAdapter #21108

Conversation

nicolas-grekas commented Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

robfrawley Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley commented Dec 30, 2016 • edited Loading

robfrawley commented Dec 30, 2016

nicolas-grekas commented Dec 31, 2016

robfrawley commented Dec 31, 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

robfrawley Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Jan 3, 2017

robfrawley commented Jan 3, 2017 • edited Loading

nicolas-grekas commented Jan 4, 2017

nicolas-grekas commented Jan 4, 2017

nicolas-grekas commented Dec 30, 2016 •

edited

Loading

robfrawley Dec 30, 2016 •

edited

Loading

robfrawley Dec 30, 2016 •

edited

Loading

robfrawley Dec 30, 2016 •

edited

Loading

robfrawley Dec 30, 2016 •

edited

Loading

robfrawley Dec 30, 2016 •

edited

Loading

nicolas-grekas Dec 30, 2016 •

edited

Loading

robfrawley commented Dec 30, 2016 •

edited

Loading

robfrawley commented Dec 31, 2016 •

edited

Loading

robfrawley Dec 31, 2016 •

edited

Loading

robfrawley commented Jan 3, 2017 •

edited

Loading