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

[WIP] Memcached #2770

Closed
wants to merge 1 commit into from
Closed

[WIP] Memcached #2770

wants to merge 1 commit into from

Conversation

marc-mabe
Copy link
Member

  • removed dependency to Zend\Validator
  • allow to share one memcached resource over different adapter instances
  • instantiate the memcached resource on first use
  • init list of servers and lib options only on instantiating the memcached resource
  • allow different format to configure servers:
    • array list: array(array([, [, ]][, ...]))
    • array assoc: array(array('host' => [, 'port' => [, 'weight' => ]][, ...]))
    • string list: array(':?weight='[, ...])
    • string separated: ':?weight=[, ...]'

 - removed dependency to Zend\Validator
 - allow to share one memcached resource over differend adapter instances
 - instantiate the memcached resource on first use
 - init list of servers and lib options only on instantiating the memcached resource
*/
public function addServer($host, $port = 11211)
public function setMemcachedResource(MemcachedResource $memcachedResource = null)
Copy link
Member

Choose a reason for hiding this comment

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

This is not good -- it's an API change. I'm okay with adding a method, but not with removing one.

weierophinney added a commit that referenced this pull request Oct 16, 2012
- Re-instated MemcachedOptions::addServer(), and added optional $weight argument
  (defaults to 0, the default used by Memcached)
- Re-instated default server
- Re-instated ability to change namespaces
- Re-instated removed tests
- Fixed failing tests, and corrected assumptions in several tests (as internal
  implementation details had changed)
weierophinney added a commit that referenced this pull request Oct 16, 2012
@weierophinney
Copy link
Member

All suggestions were incorporated during merge. I re-added the listener for "option", but have it now only caring about "namespace"; this fixed two failing unit tests. Additionally, I'd discovered that the default server had been removed, so I reinstated that as well, this time within MemcachedOptions.

@marc-mabe
Copy link
Member Author

Hi @weierophinney

Thank you for review and corrections.
But saw some problems with it but couldn't fix it all because of stress at work and now I'm sick :(
(Sry if I didn't communicate)

  1. addServer: The event option should be triggered on changing an option but currently doesn't for servers and if it will be implemented it would for every server instead of only once for all.
  2. Noted in Missing expiration parameter for "setItem" on Cache adapters #2760 it must be possible to share the same memcached resource over different adapter instances but than there are always servers and lib options configured. It's than would add the default configured server or the servers have to be configured twice and would be added twice.
  3. Similar as 2.
    It must be possible to share the same memcached instances but use different namespaces.
    • I would like to implement the namespace support manually and let it be independent of Memcached::OPT_PREFIX_KEY (It wasn't ready)

Any thoughts ?

@weierophinney
Copy link
Member

@marc-mabe :

  1. I attached the option listener directly to the memcached resource, which means it should affect all servers (as servers are attached to the memcached resource, and they cannot be changed once the resource is initialized). As such, I think this is working correctly now.
  2. I think that if you use the same memcached resource over different adapter instances, you're asking for problems, unless you know that the exact same configuration will work.
  3. Same thing -- different namespaces will lead to liklihood of collisions. My inclination is that this is a YAGNI situation. (That said, the liklihood of namespace issues should be moot, as if you provide the namespace in an operation, the listener will be triggered, and thus change the namespace for that operation.)

weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015
- Re-instated MemcachedOptions::addServer(), and added optional $weight argument
  (defaults to 0, the default used by Memcached)
- Re-instated default server
- Re-instated ability to change namespaces
- Re-instated removed tests
- Fixed failing tests, and corrected assumptions in several tests (as internal
  implementation details had changed)
weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015
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.

None yet

2 participants