Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Zend\Cache\Storage\Adapter\Memcache #5829

Closed
wants to merge 9 commits into from

Conversation

cgmartin
Copy link
Contributor

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

Implements feature request: #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,
            ),
        ),
    ),
));


namespace Zend\Cache\Storage\Adapter;

use Memcache as MemcacheResource;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused use Memcache

@cgmartin
Copy link
Contributor Author

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

@samsonasik
Copy link
Contributor

👍

* @return array Array of not stored keys
* @throws Exception\ExceptionInterface
*/
protected function internalSetItems(array & $normalizedKeyValuePairs)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

Only the above comments found.
Looks good - thanks !

@cgmartin
Copy link
Contributor Author

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.

@cgmartin
Copy link
Contributor Author

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

@marc-mabe
Copy link
Member

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 :/

* @param MemcacheResource $resource
* @param array $libOptions
*/
protected function setResourceLibOptions(MemcacheResource $resource, array $libOptions)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@cgmartin
Copy link
Contributor Author

@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 weierophinney added this to the 2.3.0 milestone Mar 3, 2014
@weierophinney weierophinney self-assigned this Mar 5, 2014
weierophinney added a commit that referenced this pull request Mar 5, 2014
Zend\Cache\Storage\Adapter\Memcache
weierophinney added a commit that referenced this pull request Mar 5, 2014
- Promoted contents of several conditional statements, so that the bulk of the
  methods' work does not happen inside conditionals.
weierophinney added a commit that referenced this pull request Mar 5, 2014
@weierophinney
Copy link
Member

Merged to develop for release with 2.3.0.

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

Zend\Cache\Storage\Adapter\Memcache
weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015
- Promoted contents of several conditional statements, so that the bulk of the
  methods' work does not happen inside conditionals.
weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants