Opened 5 years ago

Last modified 2 weeks ago

#15815 new New feature

Support memcached binary protocol in PyLibMCCache

Reported by: mtigas Owned by:
Component: Core (Cache system) Version: 1.3
Severity: Normal Keywords: sprint2013
Cc: brandon.konkle@…, rob@…, mlowicki@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

As per http://sendapatch.se/projects/pylibmc/#example-usage , enabling the binary protocol requires the binary=True argument to be passed into pylibmc.Client when it is initialized.

The binary protocol is available in Memcached 1.3+ and provides a performance boost in high-load circumstances: http://www.slideshare.net/tmaesaka/memcached-binary-protocol-in-a-nutshell-presentation

pylibmc ignores any unknown options passed into Client.behaviors (see below), so putting this in OPTIONS seems like the way to go.

>>> import pylibmc
>>> a=pylibmc.Client(['127.0.0.1:55838',])
>>> print a.behaviors
{'cas': 0, 'no_block': 0, 'receive_timeout': 0, 'send_timeout': 0, 'ketama_hash': 0, '_poll_timeout': 5000, '_io_bytes_watermark': 66560, 'cache_lookups': 0, '_sort_hosts': 0, '_io_key_prefetch': 0, '_auto_eject_hosts': 0, 'ketama': 0, 'ketama_weighted': 0, '_io_msg_watermark': 500, '_hash_with_prefix_key': 0, 'tcp_nodelay': 0, 'failure_limit': 0, 'buffer_requests': 0, '_socket_send_size': 0, '_retry_timeout': 0, '_noreply': 0, '_socket_recv_size': 0, '_number_of_replicas': 0, 'distribution': 'modula', 'connect_timeout': 4000, 'hash': 'default', 'verify_keys': 0}

>>> a.behaviors = {"binary":True} # should be ignored by pylibmc
>>> print a.behaviors
{'cas': 0, 'no_block': 0, 'receive_timeout': 0, 'send_timeout': 0, 'ketama_hash': 0, '_poll_timeout': 5000, '_io_bytes_watermark': 66560, 'cache_lookups': 0, '_sort_hosts': 0, '_io_key_prefetch': 0, '_auto_eject_hosts': 0, 'ketama': 0, 'ketama_weighted': 0, '_io_msg_watermark': 500, '_hash_with_prefix_key': 0, 'tcp_nodelay': 0, 'failure_limit': 0, 'buffer_requests': 0, '_socket_send_size': 0, '_retry_timeout': 0, '_noreply': 0, '_socket_recv_size': 0, '_number_of_replicas': 0, 'distribution': 'modula', 'connect_timeout': 4000, 'hash': 'default', 'verify_keys': 0}

Attachments (1)

pylibmc-binary.diff (656 bytes) - added by mtigas 5 years ago.

Download all attachments as: .zip

Change History (18)

Changed 5 years ago by mtigas

comment:1 Changed 5 years ago by mtigas

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Not sure if patch is in the proper format; have also opened a github pull request, tracking this bug. https://github.com/django/django/pull/21

Looked into updating the docs, however the documentation appears to leave out implementation-specific details of the different memcached backends. Please let me know if I’m mistaken.

comment:2 Changed 5 years ago by jacob

  • Easy pickings unset
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

The patch isn't ideal - special-casing "binary" is a bit annoying - but this is totally worth having.

comment:3 Changed 5 years ago by mtigas

I’ll agree with that, though I wasn’t sure how to make this semantically better.

For the older cache framework, django-newcache accepted binary as a cache param (with timeout, cull_frequency, et. al.), with CACHE_BEHAVIORS as a separate settings option. (Using django-newcache as a comparison point since some of the newer 1.3+ cache features like versioning and key prefixing appear to have been based on the newcache implementation.)

But I didn’t quite feel that special casing PyLibMCCache to accept a new base parameter was correct, either …

CACHES = {
    'default' : {
        "BACKEND" : 'django.core.cache.backends.memcached.PyLibMCCache',
        "LOCATION" : '127.0.0.1:11211',
        "BINARY" : True,
        "OPTIONS" : dict(tcp_nodelay=True, ketama=True),
    }
}

… since the description of OPTIONS reads, “Any options that should be passed to cache backend. The list options understood by each backend vary with each backend. […] Cache backends backed by a third-party library will pass their options directly to the underlying cache library.”

In particular, that seems to imply that for consistency’s sake, all implementation-specific options regarding a backend should go into OPTIONS and that it’s up to the backend to do what it needs to provide the correct information to the underlying library.

Technically, a more semantically-correct option would be to do something like:

CACHES = {
    'default' : {
        "BACKEND" : 'django.core.cache.backends.memcached.PyLibMCCache',
        "LOCATION" : '127.0.0.1:11211',
        "OPTIONS" : {
            "binary": True,
            "behaviors" : dict(tcp_nodelay=True, ketama=True),
        }
    }
}

Not really sure what the best patch would be at this point.

comment:4 Changed 5 years ago by bkonkle

  • Cc brandon.konkle@… added
  • UI/UX unset

comment:5 Changed 4 years ago by robhudson

  • Cc rob@… added

I recently updated django-pylibmc, a 3rd party cache backend, with a few features we wanted at Mozilla. Namely, to make a timeout of zero mean an infinite timeout, add compression support (which pylibmc handles), and support the binary protocol. The package can be found here: https://github.com/jbalogh/django-pylibmc.

I'd be happy to help push this forward or bring some code over into Django. Mapping OPTIONS to pylibmc "behaviors" and adding the extra BINARY=True parameter made sense to me. But whichever way is decided, this should be an easy thing to add to this backend.

comment:6 Changed 3 years ago by aaugustin

#19810 is related.

comment:7 Changed 3 years ago by bpeschier

  • Keywords sprint2013 added

Enabling binary mode breaks the tests at the moment, because keys with spaces are actually allowed in binary mode.

comment:8 Changed 3 years ago by bpeschier

  • Owner changed from nobody to bpeschier
  • Status changed from new to assigned

Working on it here: https://github.com/bpeschier/django/tree/ticket_15815

Still needs some docs to demystify the options for pylibmc a bit. Currently all options are set as behaviors except binary (which still feels a bit ugly)

comment:9 Changed 3 years ago by otherjacob

I really don't think special casing is a big deal here -- it's not far off of what BaseCache does already, although I'd rather pop binary off of _options in the _cache method before setting behaviors to it.

Happy to finish this off if bpeschier can't get to it, but it's almost done at this point I think.

comment:10 Changed 2 years ago by the_drow

So this is already implemented in https://github.com/jbalogh/django-pylibmc.
Shouldn't we just merge that cache provider back?

comment:11 Changed 11 months ago by bpeschier

  • Owner bpeschier deleted
  • Status changed from assigned to new

comment:12 Changed 7 months ago by thedrow

How come this is not supported? What's blocking it exactly?
How can I help?

comment:13 Changed 7 months ago by timgraham

I don't see a reviewable patch.

comment:14 Changed 2 months ago by mlowicki

any news about it?

comment:15 Changed 2 months ago by mlowicki

  • Cc mlowicki@… added

comment:16 Changed 2 weeks ago by edmorley

In case this helps anyone working on this in the future, if you get test failures running the Django pylibmc cache tests with binary mode enabled in test_zero_timeout() - I believe it's due to a bug in older versions of libmemcached-dev.

This appears to only affect .set() and not .add(), and only appears if using binary mode. The bug means that a Django zero timeout (which is converted to -1 when passed to pylibmc) is ignored by libmemcached and the key is in fact set after all.

Affected libmemcached versions include that running on Travis on their Ubuntu precise images (which presuming they are using the official package is http://packages.ubuntu.com/precise/libmemcached-dev). libmemcached-dev 1.0.8 on Ubuntu trusty works fine. I couldn't find a related bug or commit on https://launchpad.net/libmemcached but Launchpad is pretty painful to use so I may have just missed it.

comment:17 Changed 2 weeks ago by edmorley

Correction: it affects both set() and add() from what I can now tell. The behaviour also seems inconsistent on newer versions too.

I've opened an issue against pylibmc for this:
https://github.com/lericson/pylibmc/issues/202

Note: See TracTickets for help on using tickets.
Back to Top