Code

Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#6464 closed (fixed)

memcached cache backend incr/decr not implemented

Reported by: panni@… Owned by: nobody
Component: Core (Cache system) Version: master
Severity: Keywords: cache memcached incr decr
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The very efficient memcached.incr/decr() functions aren't implemented in django.core.cache.backends.memcached.CacheClass,
but accessible through cache._cache.incr/decr()

Attachments (2)

6464.memcached.incr.decr.diff (973 bytes) - added by PJCrosier 6 years ago.
Adds decr and incr support to the memcached backend
6464.memcached.incr.decr.2.diff (4.2 KB) - added by mmalone 6 years ago.
Increment and decrement operations for cache backends

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by Simon Greenhill <dev@…>

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

Changed 6 years ago by PJCrosier

Adds decr and incr support to the memcached backend

comment:2 Changed 6 years ago by PJCrosier

  • Has patch set
  • Keywords cache memcached incr added; cache, memcached, incr, removed
  • Needs documentation set
  • Needs tests set

Patch attached that adds decr and incr support to the memcached backend - if the key doesn't exist, the value of the key isn't an integer or the delta isn't an integer then None is returned.

comment:3 Changed 6 years ago by PJCrosier

Heh, I should explain my own code better - if the key doesn't exist then return None, if the key's value and the delta are both integers then return the key's new value, otherwise return the key's value.

Changed 6 years ago by mmalone

Increment and decrement operations for cache backends

comment:4 Changed 6 years ago by mmalone

  • milestone set to post-1.0

New patch is a simple wrapper around the memcache library for the memcached backend. It also includes a simple (non-atomic) implementation for other cache backends, and raises a NotImplementedException for the dummy cache backend (we can't return the incremented/decremented value since we don't have the original value).

Since non-memcache backends can't perform this operation atomically, it might be better to raise a NotImplementedException for all non-memcache backends. That said, I added docs that make it pretty clear that these operations are only atomic for memcached...

comment:5 Changed 6 years ago by mmalone

  • Needs documentation unset
  • Needs tests unset

comment:6 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:7 Changed 5 years ago by AdamG

  • Triage Stage changed from Accepted to Design decision needed

Marking as DDN: it's expanding the scope django.core.cache, and the unavoidable inconsistencies in the cache backends might make this something outside of Django's scope.

After all, incr/decr are not general caching primitives; they're memcached primitives. I'm not sure we should try to simulate them on other backends.

comment:8 Changed 5 years ago by jacob

  • milestone set to 1.1 beta
  • Triage Stage changed from Design decision needed to Accepted

Thing is, though, that incr/decr are so ridiculously useful on memcached that I think not supporting them effectively cripples our cache support. Yes, emulating them on other backends makes those operations slower for those backends, but the fact is that if you want speed you should be using memcached, anyway. Thus, I'm going to make the design decision and accept this.

comment:9 Changed 5 years ago by mmalone

I agree w/ Jacob, and feel the same way about much of the memcache API. While some of the operations may not be "general" caching primitives, nearly all of them are extremely useful caching primitives. The memcache API is pretty simple, and if memcache implements a particular API and Django doesn't, I think it's worth seriously questioning why.

comment:10 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [10031]) Fixed #6464 -- Added incr() and decr() operations on cache backends. Atomic on Memcache; implemented as a 2 stage retrieve/update on other backends. Includes refactor of the cache tests to ensure all the backends are actually tested, and a fix to the DB cache backend that was discovered as a result. Thanks to Michael Malone for the original patch.

comment:11 Changed 3 years ago by jacob

  • milestone 1.1 beta deleted

Milestone 1.1 beta deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.