It might need some love with tweaking the time values but other than it's good.

CommentFileSizeAuthor
#10 memcache.patch1.99 KBJeremy
#4 stampede.patch1.82 KBchx
m.patch1.3 KBchx
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

chx’s picture

Title: Cache stamped protection » Cache stampede protection
chx’s picture

Here is why this patch is good: imagine you have N frontends and a memcache pool and you cache the results of a slow query. Now, when that object expires then you will suddenly run N pieces of slow query. With this patch you will run said query before the object can expire and you are guaranteed to run only one.

Edit: to achieve this, we will return one and exactly one FALSE 30 seconds before the object can expire so that the requester will recreate and recache the object.

robertDouglass’s picture

Thanks for finally providing the patch!

chx’s picture

FileSize
1.82 KB

This is an even better version. The previous version had a problem: if the object expired then you still could get a stampede. So now all the items are permanent and once the item has expired, one and only one process will get a chance to re-build and re-set the object. Ample comments are provided because it's not entirely trivial but IMO it's rather elegant.

Jeremy’s picture

For any content that expires with a TTL, I like this very much. But this doesn't help with any content that expires with a call to cache_clear_all, does it?

Jeremy’s picture

Status: Needs review » Reviewed & tested by the community

The cache_clear_all issue is secondary -- this patch is really just trying to address items that expire after a specific ttl. In my opinion, this is very clean and should be committed. Does anyone else want to weigh in on it?

Silence will be construed as a yes vote.

robertDouglass’s picture

I've followed this with chx from near the beginning of the idea. Thumbs up from me.

Jeremy’s picture

Status: Reviewed & tested by the community » Needs work

@chx: Do we really want to hold the lock for 30 seconds? Why do we need to hold the lock longer than 5 seconds? Perhaps we should turn this into a define() which can make it even more obvious what's going on?

chx’s picture

Jeremy: whatever. I can't think of a scenario where you would cache an object for less than 30s and then again a rebuild process that takes more than 30s is horrible... I can't say the same thing for 5s -- if a rebuild takes 5.5s, well we are not happy but it's not the end of the world.

Jeremy’s picture

Status: Needs work » Needs review
FileSize
1.99 KB

Chx: How is the attached? I compromised per our irc discussion to 15 seconds, and made it a define. I also reworded some comments (but it's quite late as I do this).

chx’s picture

I rather dislike the word "session", you wanted "request". "we do not return the cached object, instead causing it to be rebuilt" what about "return a FALSE isntead of the cache object causing it to be rebuilt"?

David Strauss’s picture

CACHE_STAMPEDE_SEMAPHORE should probably be a variable to be more easily override-able.

I dislike telling memcache that the expiration for items is infinite. I'm guessing it hurts the ability for memcache to effectively prune a full cache.

Jeremy’s picture

Status: Needs review » Active

Committed to 6.x-1.x-dev. Will backport to 5.x-dev as well.

Jeremy’s picture

Per David's request, made memcache_stampede_semaphore a variable (in 6.x-1.x-dev).

Jeremy’s picture

Status: Active » Fixed

Backported this patch and committed to 5.x-1.x-dev.

Jeremy’s picture

Note, David and I discussed his concern about memcache's LRU, and we confirmed that the lifetime of the cached item does not affect memcache's LRU algorithm's ability to prune content when it fills up.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.