Zend\Cache\Storage\Adapter\Memcache #5829

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

cgmartin commented Feb 15, 2014

New cache storage adapter for ext/memcache (not to be confused with existing ext/memcached adapter).

Implements feature request: zendframework#4481

An example configuration for Zend\Cache\StorageFactory would be:

$memcacheStorageAdapter = \Zend\Cache\StorageFactory::factory(array(
    'adapter' => array(
        'name' => 'memcache',
        'options' => array(
            'compression' => false, // use compression on all writes when able?
            'auto_compress_threshold' => 100,   // compression threshold options
            'auto_compress_min_savings' => 0.2,            
            'servers' => array(
                0 => array('host' => '10.0.0.20', 'port' => 11211), // shorthand, server_defaults will be applied
                1 => array('host' => '10.0.0.21', 'port' => 11211, 'status' => true), 
                2 => array('host' => '10.0.0.22', 'port' => 11211, 'status' => true, 'weight' => 3, 
                           'persistent' => false, 'timeout' => 2, 'retry_interval' => 20), // or can specify all values
            ),
            'server_defaults' => array( 
                // defaults that are applied to servers with missing values
                'weight' => 1,
                'persistent' => true,
                'timeout' => 1,
                'retry_interval' => 15,
            ),
        ),
    ),
));

@samsonasik samsonasik commented on an outdated diff Feb 15, 2014

library/Zend/Cache/Storage/Adapter/MemcacheOptions.php
@@ -0,0 +1,266 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2014 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\Cache\Storage\Adapter;
+
+use Memcache as MemcacheResource;
@samsonasik

samsonasik Feb 15, 2014

Contributor

unused use Memcache

@samsonasik samsonasik commented on an outdated diff Feb 15, 2014

tests/ZendTest/Cache/Storage/Adapter/MemcacheTest.php
@@ -0,0 +1,134 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
@samsonasik

samsonasik Feb 15, 2014

Contributor

now 2014

@samsonasik samsonasik commented on an outdated diff Feb 15, 2014

...Cache/Storage/Adapter/MemcacheResourceManagerTest.php
@@ -0,0 +1,352 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
@samsonasik

samsonasik Feb 15, 2014

Contributor

2014

@samsonasik samsonasik commented on an outdated diff Feb 15, 2014

...Cache/Storage/Adapter/MemcacheResourceManagerTest.php
+ return $data;
+ }
+
+ /**
+ * @dataProvider validResourceProvider
+ * @param string $resourceId
+ * @param mixed $resource
+ * @param array $expectedServers
+ * @param array $expectedLibOptions
+ */
+ public function testValidResources($resourceId, $resource, $expectedServers, $expectedLibOptions)
+ {
+ // php-memcache is required to set libmemcache options
+ if (is_array($resource) && isset($resource['lib_options']) && count($resource['lib_options']) > 0) {
+ if (!class_exists('Memcache', false)) {
+ $this->setExpectedException('Zend\Cache\Exception\InvalidArgumentException', 'Unknown libmemcached option');
@samsonasik

samsonasik Feb 15, 2014

Contributor

libmemcache without d at the end ?

Contributor

cgmartin commented Feb 15, 2014

Thanks for the review, @samsonasik. I think I've gotten everything buttoned up now. Let me know if I've missed anything.

Contributor

samsonasik commented Feb 15, 2014

👍

@marc-mabe marc-mabe and 1 other commented on an outdated diff Feb 17, 2014

library/Zend/Cache/Storage/Adapter/Memcache.php
+
+ if (!$memc->set($this->namespacePrefix . $normalizedKey, $value, $flag, $expiration)) {
+ throw new Exception\RuntimeException('Memcache set value failed');
+ }
+
+ return true;
+ }
+
+ /**
+ * Internal method to store multiple items.
+ *
+ * @param array $normalizedKeyValuePairs
+ * @return array Array of not stored keys
+ * @throws Exception\ExceptionInterface
+ */
+ protected function internalSetItems(array & $normalizedKeyValuePairs)
@marc-mabe

marc-mabe Feb 17, 2014

Member

This method could be dropped as the memcache extension doesn't support setting items as bulk and setting it as a loop should be done already by the abstract adapter.

@cgmartin

cgmartin Feb 17, 2014

Contributor

Thanks. I think I was going cross-eyed from comparing the Memcache/Memcached documentation and was looking at the wrong setMulti page at the time. Will fix.

@marc-mabe marc-mabe commented on an outdated diff Feb 17, 2014

library/Zend/Cache/Storage/Adapter/Memcache.php
+ *
+ * @param string $normalizedKey
+ * @param int $value
+ * @return int|bool The new value on success, false on failure
+ * @throws Exception\ExceptionInterface
+ */
+ protected function internalIncrementItem(& $normalizedKey, & $value)
+ {
+ $memc = $this->getMemcacheResource();
+ $internalKey = $this->namespacePrefix . $normalizedKey;
+ $value = (int) $value;
+ $newValue = $memc->increment($internalKey, $value);
+
+ if ($newValue === false) {
+ // Set initial value. Don't use compression!
+ // http://us3.php.net/manual/en/memcache.increment.php
@marc-mabe

marc-mabe Feb 17, 2014

Member

Please remove the localized URL parts us3 & /en

@marc-mabe marc-mabe commented on an outdated diff Feb 17, 2014

library/Zend/Cache/Storage/Adapter/Memcache.php
+ * Internal method to decrement an item.
+ *
+ * @param string $normalizedKey
+ * @param int $value
+ * @return int|bool The new value on success, false on failure
+ * @throws Exception\ExceptionInterface
+ */
+ protected function internalDecrementItem(& $normalizedKey, & $value)
+ {
+ $memc = $this->getMemcacheResource();
+ $internalKey = $this->namespacePrefix . $normalizedKey;
+ $value = (int) $value;
+ $newValue = $memc->decrement($internalKey, $value);
+
+ if ($newValue === false) {
+ // Set initial value
@marc-mabe

marc-mabe Feb 17, 2014

Member

Should have the same comments as increment

@marc-mabe marc-mabe and 1 other commented on an outdated diff Feb 17, 2014

library/Zend/Cache/Storage/Adapter/Memcache.php
+ /**
+ * Internal method to get capabilities of this adapter
+ *
+ * @return Capabilities
+ */
+ protected function internalGetCapabilities()
+ {
+ if ($this->capabilities === null) {
+ $this->capabilityMarker = new stdClass();
+ $this->capabilities = new Capabilities(
+ $this,
+ $this->capabilityMarker,
+ array(
+ 'supportedDatatypes' => array(
+ 'NULL' => true,
+ 'boolean' => 'boolean',
@marc-mabe

marc-mabe Feb 17, 2014

Member

As the memcache API returns false on error and there is no way the check if the return value false was an error of a valid cached value it's not possible to store booleans.

@cgmartin

cgmartin Feb 17, 2014

Contributor

...not possible to store booleans

This doesn't seem to be true in my tests. It seems that you cannot store booleans as boolean type, but the extension will successfully serialize them to a string:
https://gist.github.com/cgmartin/9052704

Setting the capability to (string) 'boolean' also passes the capability test here:
https://github.com/zendframework/zf2/blob/master/tests/ZendTest/Cache/Storage/Adapter/CommonAdapterTest.php#L634-637

I'm no expert with Memcache so just let me know if I'm not understanding this correctly.

@marc-mabe

marc-mabe Feb 17, 2014

Member

The code from internalGetItem:

        $result = $memc->get($internalKey);
        $success = ($result !== false);
        if ($result === false) {
            return null;
        }
        $casToken = $result;
        return $result;

... so FALSE means that the item doesn't exist.

What is the result of the following ?:

$cache->setItem('test', false);
var_dump($cache->getItem('test', $success));
var_dump($success);
@cgmartin

cgmartin Feb 17, 2014

Contributor

Same result as my standalone test gist, it serialized to a string when written:

string(0) ""  // getItem
bool(true)    // $success

It's up to the user to cast to boolean. Is that the issue? Numbers (int, float) get serialized to strings as well, it seems.

@cgmartin

cgmartin Feb 17, 2014

Contributor

Hold on a sec...
I just retested on a different computer and I'm seeing what you expected eariler:

NULL         // getItem
bool(false)  // $success

This computer is running PHP 5.4.21 with ext/memcache version 3.0.8.

I'll check tonight what exactly my other computer was running (it was PHP 5.5). Very odd.

@cgmartin

cgmartin Feb 17, 2014

Contributor

It appears there are differences in behavior between the 2.x stable versions of ext/memcache and 3.x devel versions, specifically in 3.0.3:

Scalar data types (int, bool, double) are preserved by get/set

I'm going to run some more tests using different versions to iron this out.

If I can confirm the version that changes this behavior, we might be able to return a different compatibility spec based on the installed version (is my current thought). Details to come...

@marc-mabe

marc-mabe Feb 18, 2014

Member

It looks like since ext/memcache-3.0.3 the extension serializes and unserializes the given values. So the capabilities you provided for >=3.0.3 are OK but the one for lower versions have to be the same as for other not serializing storages (https://github.com/zendframework/zf2/blob/master/library/Zend/Cache/Storage/Adapter/Filesystem.php#L1153) ... Did you test array and object ?

@cgmartin

cgmartin Feb 18, 2014

Contributor

For the lower 2.x versions, I've tested all of the capability data types, including NULL, array and object. The capability tests in CommonAdapterTest all pass for both 3.x and 2.x versions.

@marc-mabe marc-mabe commented on an outdated diff Feb 17, 2014

...end/Cache/Storage/Adapter/MemcacheResourceManager.php
+ *
+ * @var array
+ */
+ protected $resources = array();
+
+ /**
+ * Default server values per resource
+ *
+ * @var array
+ */
+ protected $serverDefaults = array();
+
+ /**
+ * Failure callback per resource
+ *
+ * @var array
@marc-mabe

marc-mabe Feb 17, 2014

Member

@var callable[]

@marc-mabe marc-mabe and 1 other commented on an outdated diff Feb 17, 2014

tests/ZendTest/Cache/Storage/Adapter/MemcacheTest.php
+
+use Zend\Cache;
+
+/**
+ * @group Zend_Cache
+ */
+class MemcacheTest extends CommonAdapterTest
+{
+
+ public function setUp()
+ {
+ if (!defined('TESTS_ZEND_CACHE_MEMCACHE_ENABLED') || !TESTS_ZEND_CACHE_MEMCACHE_ENABLED) {
+ $this->markTestSkipped("Skipped by TestConfiguration (TESTS_ZEND_CACHE_MEMCACHE_ENABLED)");
+ }
+
+ if (!extension_loaded('memcache')) {
@marc-mabe

marc-mabe Feb 17, 2014

Member

If the extension isn't loaded we should test if the exception will be thrown - like it's done in the APC adapter: https://github.com/zendframework/zf2/blob/master/tests/ZendTest/Cache/Storage/Adapter/ApcTest.php#L33
(I have seen it will not be done in the memcached adapter but it should)

@cgmartin

cgmartin Feb 17, 2014

Contributor

Ah, I follow. Will do. Thanks!

Member

marc-mabe commented Feb 17, 2014

Only the above comments found.
Looks good - thanks !

Contributor

cgmartin commented Feb 17, 2014

Thanks @marc-mabe for the review. I think I've got all of your comments addressed, except for the one about booleans that I've replied to. Let me know if there's anything else I've missed. Thanks again.

Contributor

cgmartin commented Feb 18, 2014

Finished testing differences between ext/memcache 3.x beta and 2.x stable versions, which affects the adapter capabilities: https://github.com/cgmartin/zf2/blob/2c1649e9ca8b7683cd10ad0e65e178a60af49cf5/library/Zend/Cache/Storage/Adapter/Memcache.php#L481-517

Member

marc-mabe commented Feb 18, 2014

I had commented on the method internalRemoveItem to add the second argument to delete fixed as 0 because there was a bug somewhere - My comment goes away but nothing changed yet :/

@marc-mabe marc-mabe and 1 other commented on an outdated diff Feb 18, 2014

...end/Cache/Storage/Adapter/MemcacheResourceManager.php
+ $this->normalizeCompressThresholdOptions($value);
+ break;
+ }
+ $result[$key] = $value;
+ }
+
+ $libOptions = $result;
+ }
+
+ /**
+ * Set lib options on a Memcache resource
+ *
+ * @param MemcacheResource $resource
+ * @param array $libOptions
+ */
+ protected function setResourceLibOptions(MemcacheResource $resource, array $libOptions)
@marc-mabe

marc-mabe Feb 18, 2014

Member

The lib_options was in the memcached adapter to proxy to Memcached::setOption which changes from version to version. In my opinion it isn't needed for ext/memcache and it would be better to make it a normal getter/setter [get|set]AutoCompressThreshold and [get|set]AutoCompressMinSaving

@cgmartin

cgmartin Feb 18, 2014

Contributor

Yeah, I can empathize with that. My original thought was to try make it similar to the Memcached adapter config, but at second glance it just seems awkward here. Will do.

Contributor

cgmartin commented Feb 18, 2014

@marc-mabe regarding the delete second argument, I looked back through your comments and didn't see anything mentioned about this previously. I found what you are talking about, though, and will make that change:

Delete's second parameter (timeout) is deprecated and not supported.
Values other than 0 may cause delete to fail.
http://www.php.net/manual/memcache.delete.php

weierophinney added this to the 2.3.0 milestone Mar 3, 2014

weierophinney self-assigned this Mar 5, 2014

@weierophinney weierophinney added a commit that referenced this pull request Mar 5, 2014

@weierophinney weierophinney Merge pull request #5829 from cgmartin/memcache-adapter
Zend\Cache\Storage\Adapter\Memcache
c68ca3a

@weierophinney weierophinney added a commit that referenced this pull request Mar 5, 2014

@weierophinney weierophinney [#5829] object calisthenics
- Promoted contents of several conditional statements, so that the bulk of the
  methods' work does not happen inside conditionals.
cb909b4

@weierophinney weierophinney added a commit that referenced this pull request Mar 5, 2014

@weierophinney weierophinney Merge branch 'feature/5829' into develop
Close #5829
Fixes #4481
99d5d41
Owner

weierophinney commented Mar 5, 2014

Merged to develop for release with 2.3.0.

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

@weierophinney weierophinney Merge pull request zendframework/zendframework#5829 from cgmartin/mem…
…cache-adapter

Zend\Cache\Storage\Adapter\Memcache
d93eda8

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

@weierophinney weierophinney [zendframework/zendframework#5829] object calisthenics
- Promoted contents of several conditional statements, so that the bulk of the
  methods' work does not happen inside conditionals.
7701c54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment