Opened 7 years ago

Closed 23 months ago

Last modified 23 months ago

#15815 closed New feature (duplicate)

Support memcached binary protocol in PyLibMCCache

Reported by: Mike Tigas Owned by:
Component: Core (Cache system) Version: 1.3
Severity: Normal Keywords: sprint2013
Cc: brandon.konkle@…, rob@…, mlowicki@…, emorley@… 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 Mike Tigas 7 years ago.

Download all attachments as: .zip

Change History (19)

Changed 7 years ago by Mike Tigas

Attachment: pylibmc-binary.diff added

comment:1 Changed 7 years ago by Mike Tigas

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 7 years ago by Jacob

Easy pickings: unset
Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

comment:3 Changed 7 years ago by Mike Tigas

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 7 years ago by Brandon Konkle

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

comment:5 Changed 7 years ago by Rob Hudson

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 5 years ago by Aymeric Augustin

#19810 is related.

comment:7 Changed 5 years ago by Bas Peschier

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 5 years ago by Bas Peschier

Owner: changed from nobody to Bas Peschier
Status: newassigned

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 5 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 5 years ago by Omer Katz

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

comment:11 Changed 3 years ago by Bas Peschier

Owner: Bas Peschier deleted
Status: assignednew

comment:12 Changed 3 years ago by Omer Katz

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

comment:13 Changed 3 years ago by Tim Graham

I don't see a reviewable patch.

comment:14 Changed 3 years ago by Michał Łowicki

any news about it?

comment:15 Changed 3 years ago by Michał Łowicki

Cc: mlowicki@… added

comment:16 Changed 2 years ago by Ed Morley

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.

Version 0, edited 2 years ago by Ed Morley (next)

comment:17 Changed 2 years ago by Ed Morley

<obsolete comment>

Last edited 23 months ago by Ed Morley (previous) (diff)

comment:18 Changed 23 months ago by Ed Morley

Cc: emorley@… added
Resolution: duplicate
Status: newclosed

This has been fixed in ticket #20892, by adding generic support for passing parameters through to the memcached client constructor (and for all backends, not just PyLibMCCache).

pylibmc's constructor is:

    def __init__(self, servers, behaviors=None, binary=False,
                 username=None, password=None):

So on Django master (will be 1.11), binary mode can be enabled using eg:

CACHES = {
    'default': {
        'BACKEND': 'django.core.cache.backends.memcached.PyLibMCCache',
        'LOCATION': '127.0.0.1:11211',
        'OPTIONS': {
            'binary': True,
            'username': 'user',
            'password': 'pass',
            'behaviors': {
                'ketama': True,
            }
        }
    }
}

For more examples, see:
https://docs.djangoproject.com/en/dev/topics/cache/#cache-arguments

I'm going to try and backport these changes (plus ticket #27152) to django-pylibmc, so the same Django settings file will work for both, to make transitioning from "older Django+django-pylibmc backend" to "Django 1.11+ with stock backend" easier.

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