Merge lp:~lifeless/launchpad/memcache into lp:launchpad

[Robert Collins]

Remove memcache TALES support and all uses of it, fixing latency-after-update bugs.

This branch does four things.

Firstly, it removes a large number of 'band-aid' memcache applications. Our early memcache use was done by putting a slow operation behind a cache-check-or-set helper. This doesn't aid performance much, and gives users poor experiences when they are the first user to use a given page. In addition, as we had no just-in-time expiry mechanism, many of these bandaid-spots resulted in bug reports, e.g. when a user cannot see a bug they just linked to a milestone on the milestone page, or cannot see new series. I can't find the bug reports just now, but I've seen them go by :). We may find there are poorly hidden queries behind things like the front page counters. If so, we should fix them - they will already be causing timeouts (and so this patch may make them more prominent, but it will in no way introduce the bug).

Secondly, it removes a couple of pathological memcache uses, where memcache made the system slower by introducing O(N) scaling to a constant page. memcache accesses cost upwards of 3ms, sometimes as much as 20 for hits, and doing that for every object in a page with hundreds of objects rapidly consumes a lot of time - more than an optimised load+render (which we have since done) requires.

Thirdly, it removes the one remaining arguable legitimate use we have, caching the blog posts on the front page; we have a squid cache that already caches this content, and is getting lots of hits. It is possible that this will turn out to be a catastrophe, but it is more likely that poor processing of object counts on the front page is actually the cause of existing timeouts.

Doing these three things permits the fourth thing, which is to remove our inappropriately layered memcache helper (which acted as a poor http-style cache, see the bugs described in #1 above), and kills a significant amount of LoC - about 1000.

[Curtis Hovey (sinzui)]

This branch is fine to land, but I suspect the removals are incomplete. I see the TALs 'cache' expression is still registered in lp/services/memcache/configure.zcml and I think it is best that it be removed too.

I think this bug work also fixes/invalidates bug 613780, bug 678320, bug 729062, bug 760335. While these bugs are about memcached, not the tales, I do not think anything is really using memcached. memcache_client_factory is the only function left, and it is only known to our testing layers. You might as well remove lp.services.memcache.

review: Approve (code)

[Robert Collins (lifeless)]

On Mon, Jun 11, 2012 at 5:23 PM, Curtis Hovey
<email address hidden> wrote:
> Review: Approve code
>
> This branch is fine to land, but I suspect the removals are incomplete. I see the TALs 'cache' expression is still registered in lp/services/memcache/configure.zcml and I think it is best that it be removed too.
>
> I think this bug work also fixes/invalidates bug 613780, bug 678320, bug 729062, bug 760335. While these bugs are about memcached, not the tales, I do not think anything is really using memcached. memcache_client_factory is the only function left, and it is only known to our testing layers. You might as well remove lp.services.memcache.

Thanks for catching the missed registration and identifying more bugs
:). I shall fix and link appropriately.

There are two garbo jobs using memcache still, and we don't have a
sensible replacement yet. I'm inclined to leave it as a useful
facility when someone wants schemaless transient storage, which is
afterall what memcache excels at.

-Rob

[Robert Collins (lifeless)]

Have looked at those bugs; some will be invalidated for now, others only if we entirely remove memcache. I suspect a trivial persistent k:v store (table defn of id, key, value) + storm class, no logic, no interface, will be less code than the client and other paraphernalia. That will be a separate pass though.