Merge lp:~leonardr/launchpad/test-representation-cache into lp:launchpad

[Leonard Richardson]

This branch integrates version 0.9.27 of lazr.restful to create a memcached-based cache for JSON representations of entries. In tests with a populated cache, (https://dev.launchpad.net/Foundations/Webservice/Performance#Store%20representations%20in%20memcached) this reduced the time to receive a representation of a collection by about 4x.

lazr.restful takes care of putting representations in the cache and retrieving them at appropriate times. This code takes care of invalidating the cache when a Storm object changes, and turning a Storm object into a unique cache key.

I plan to make one more change to this branch: make it easy to for the LOSAs to turn the cache off if it turns out to cause problems in production. I'm not really sure how to do this, so I'm presenting the branch as is as I work on this other problem.

[Brad Crittenden (bac)]

Hi Leonard,

Thanks for this branch -- it looks very promising. A 4x increase would be great.

It looks like you haven't pushed your changes to the download cache up yet so lazr.restful 0.9.27 isn't available. Be sure to do that.

You define but don't use 'json_type' in webservice/cache.txt.

The __all__ in memcache/client.py should be on multiple lines.

It looks like there are some valid complaints from 'make lint'. Please run it and clean up the ones that are real.

review: Approve (code)

[Stuart Bishop (stub)]

On Tue, Jun 1, 2010 at 11:12 PM, Leonard Richardson
<email address hidden> wrote:

> === modified file 'lib/canonical/database/sqlbase.py'

> +    def __storm_flushed__(self):
> +        """Invalidate the web service cache."""
> +        cache = getUtility(IRepresentationCache)
> +        cache.delete(self)

This looks like a decent way of hooking in. Unfortunatly, SQLBase is
legacy - used by objects providing the SQLObject compatibility layer.
Some of our newer classes use the Storm base class directly. If there
is no way to register an on-flush hook with Storm, you will need to
create a Launchpad specific subclass of storm.Storm that provides this
hook, and update classes using the Storm base class directly to use
LPStorm.

The cache can become stale by:

1) Raw SQL, at psql command line or executed by appservers or scripts.
2) Stored procedures
3) Store.remove()
4) Using Storm for bulk updates, inserts or deletions.
5) Scripts or appservers running with non-identical [memcache] config section.
6) Scripts or appservers unable to contact memcached servers
7) Network glitches

We should probably ignore the last three. Are the first four a problem
though for the webservice? If it doesn't really matter, fine.
Otherwise we might need some ON DELETE and ON UPDATE database triggers
to keep things in sync. Hopefully this is unnecessary, as it could
will kill our performance gain.

=== added file 'lib/canonical/launchpad/pagetests/webservice/cache.txt'

+The cache starts out empty.
+
+ >>> print cache.get_by_key(key)
+ None
+
+Retrieving a representation of an object populates the cache.
+
+ >>> ignore = webservice.get("/~salgado", api_version="devel").jsonBody()
+
+ >>> cache.get_by_key(key)
+ '{...}'

Is webservice.get making a real connection to the webservice here? I
am curious if the cache is populated when the object is retrieved,
rather than when the transaction that retrieved the object commits.

=== added file 'lib/lp/services/memcache/doc/restful-cache.txt'

> +An object's cache key is derived from its Storm metadata: its database
> +table name and its primary key.

> +    >>> cache_key = cache.key_for(
> +    ...     person, 'media/type', 'web-service-version')
> +    >>> print person.id, cache_key
> +    29 Person(29,),media/type,web-service-version

Do we need the LPCONFIG here too? Or is it ok for edge & production to mix?

Hmm... perhaps IMemcacheClient should grow a real API and
automatically prepend the LPCONFIG - not sure if have a use case for
edge and production sharing data, but it is convenient to have them
share the same physical Memcached servers.

> +When a Launchpad object is modified, its JSON representations for
> +recognized web service versions are automatically removed from the
> +cache.
> +
> +    >>> person.addressline1 = "New address"
> +    >>> from canonical.launchpad.ftests import syncUpdate
> +    >>> syncUpdate(person)
> +
> +    >>> print cache.get(person, json_type, "beta", default="missing")
> +    missing
> +
> +    >>> print cache.get(person, json_type, "1.0", default="missing")
> +    missing

Should we document the cases where this doesn't happen, or is this
irrelevant to the webservice?
...

[Stuart Bishop (stub)]

review: Approve

[Leonard Richardson (leonardr)]

> 1) Raw SQL, at psql command line or executed by appservers or scripts.
> 2) Stored procedures
> 3) Store.remove()
> 4) Using Storm for bulk updates, inserts or deletions.
> 5) Scripts or appservers running with non-identical [memcache] config section.
> 6) Scripts or appservers unable to contact memcached servers
> 7) Network glitches
>
> We should probably ignore the last three. Are the first four a problem
> though for the webservice? If it doesn't really matter, fine.
> Otherwise we might need some ON DELETE and ON UPDATE database triggers
> to keep things in sync. Hopefully this is unnecessary, as it could
> will kill our performance gain.

Certainly the first four are a problem. So far we have yet to come to
terms with performance improvements that involve serving stale data from
the web service (or allowing users to use the possibly stale data they
already have).

Can we put the invalidation code in the ON DELETE/ON UPDATE trigger,
rather than in the Storm trigger? Would it hurt performance to just move
the invalidation down a layer?

> + >>> ignore = webservice.get("/~salgado", api_version="devel").jsonBody()
> +
> + >>> cache.get_by_key(key)
> + '{...}'
>
> Is webservice.get making a real connection to the webservice here? I
> am curious if the cache is populated when the object is retrieved,
> rather than when the transaction that retrieved the object commits.

An HTTP request is being constructed and through Python machinations it
is invoking the web service code. But nothing's going over the network.

The cache is populated by the web service code during the GET handling.
Cache population shouldn't have anything to do with the database.

I hope this answers your question.

> > + >>> cache_key = cache.key_for(
> > + ... person, 'media/type', 'web-service-version')
> > + >>> print person.id, cache_key
> > + 29 Person(29,),media/type,web-service-version
>
> Do we need the LPCONFIG here too? Or is it ok for edge & production to mix?
>
> Hmm... perhaps IMemcacheClient should grow a real API and
> automatically prepend the LPCONFIG - not sure if have a use case for
> edge and production sharing data, but it is convenient to have them
> share the same physical Memcached servers.

Aaaah, it's absolutely not OK for edge and production to mix data. The
JSON representations include http: links to one server or the other. I
can fix this in the branch by tacking the instance name on to the cache
key.

> Should we document the cases where this doesn't happen, or is this
> irrelevant to the webservice?
>
> store.execute(Update({Person.addressline1: 'Newer Address'},
> Person.name==person.name)) should make the cache stale.

I'd say that's relevant, and another argument for putting the cache
invalidation in a database hook rather than an ORM hook.

>
> > === added file 'lib/lp/services/memcache/restful.py'
>
> > +class MemcachedStormRepresentationCache(BaseRepresentationCache):
> > + """Caches lazr.restful representations of Storm objects in memcached."""
> > +
> > + def __init__(self):
> > + self.client = memcache_client_factory()
>
> You should be using getUtility(IMemcacheClient) here - otherwise we
> end...

[Stuart Bishop (stub)]

On Wed, Jun 2, 2010 at 7:28 PM, Leonard Richardson
<email address hidden> wrote:
>> 1) Raw SQL, at psql command line or executed by appservers or scripts.
>> 2) Stored procedures
>> 3) Store.remove()
>> 4) Using Storm for bulk updates, inserts or deletions.
>> 5) Scripts or appservers running with non-identical [memcache] config section.
>> 6) Scripts or appservers unable to contact memcached servers
>> 7) Network glitches
>>
>> We should probably ignore the last three. Are the first four a problem
>> though for the webservice? If it doesn't really matter, fine.
>> Otherwise we might need some ON DELETE and ON UPDATE database triggers
>> to keep things in sync. Hopefully this is unnecessary, as it could
>> will kill our performance gain.
>
> Certainly the first four are a problem. So far we have yet to come to
> terms with performance improvements that involve serving stale data from
> the web service (or allowing users to use the possibly stale data they
> already have).
>
> Can we put the invalidation code in the ON DELETE/ON UPDATE trigger,
> rather than in the Storm trigger? Would it hurt performance to just move
> the invalidation down a layer?

We can put the invalidation code in a trigger.

It will certainly hurt performance. How much, I'm not sure. It also
depends on how much effort we put into minimizing it. The stored
procedure will need to be invoked for every row updated or deleted.

The simplest approach would be a Python stored procedure for the
trigger that invalidates the cache in band. This would be huge
overhead, both the Python overhead and waiting for network round
trips.

A better approach would be for the trigger to notify another process
of the keys that need invalidating, and have that process do it
asynchronously. This would be less overhead than our existing Slony-I
replication triggers. There would be some lag.

An alternative, which I'd need to investigate, would be to piggy back
on the existing Slony-I replication information. Whenever a row is
updated or deleted, we generate events containing this information so
the changes can be applied on the subscriber database nodes. There
would be some lag here too, but no measurable database impact.

>> + >>> ignore = webservice.get("/~salgado", api_version="devel").jsonBody()
>> +
>> + >>> cache.get_by_key(key)
>> + '{...}'
>>
>> Is webservice.get making a real connection to the webservice here? I
>> am curious if the cache is populated when the object is retrieved,
>> rather than when the transaction that retrieved the object commits.
>
> An HTTP request is being constructed and through Python machinations it
> is invoking the web service code. But nothing's going over the network.
>
> The cache is populated by the web service code during the GET handling.
> Cache population shouldn't have anything to do with the database.
>
> I hope this answers your question.

I'm wondering if it is possible that the cache gets populated, but the
webservice request fails, causing an invalid value to get stored. If
we don't populate the cache on updates then it should be fine.

>> > === added file 'lib/lp/services/memcache/restful.py'
>>
>> > +class MemcachedStormRepresen...

[Leonard Richardson (leonardr)]

> A better approach would be for the trigger to notify another process
> of the keys that need invalidating, and have that process do it
> asynchronously. This would be less overhead than our existing Slony-I
> replication triggers. There would be some lag.
>
> An alternative, which I'd need to investigate, would be to piggy back
> on the existing Slony-I replication information. Whenever a row is
> updated or deleted, we generate events containing this information so
> the changes can be applied on the subscriber database nodes. There
> would be some lag here too, but no measurable database impact.

I'm +1 on either of these ideas. The only downside is that the invalidation algorithm will become more complex over time, as I make lazr.restful cache more things and as we create more versions of the Launchpad web service.

Here's what I mean. Right now when User(29,) is invalidated we need to remove the following three cache keys:

User(29,),application/json,beta
User(29,),application/json,1.0
User(29,),application/json,devel

That's one key for every web service version. We need to have access to a list of the currently active versions, either managed separately or obtained from getUtility(IWebServiceConfiguration).active_Versions.

You also brought up the fact that all our Launchpad instances share memcached hardware. So we need to change Launchpad's key generation to include the instance name. And we need to change the invalidation code to have a list of all instance names, and to invalidate these nine keys:

User(29,),application/json,beta,edge
User(29,),application/json,beta,staging
User(29,),application/json,beta,production
User(29,),application/json,1.0,edge
User(29,),application/json,1.0,staging
User(29,),application/json,1.0,production
User(29,),application/json,devel,edge
User(29,),application/json,devel,staging
User(29,),application/json,devel,production

In the future we may change lazr.restful to cache the WADL and/or HTML representations as well as the JSON representation. In that case there could be up to 27 cache keys for User(29,).

This isn't unmanageable, but it would be much simpler if we could tell memcached to delete "User(29,).*"

> I'm wondering if it is possible that the cache gets populated, but the
> webservice request fails, causing an invalid value to get stored. If
> we don't populate the cache on updates then it should be fine.

I'm pretty sure the cache is populated right before the representation is returned to the client. If there's an error it will happen during the representation creation. If for some reason an error happens afterwards it doesn't change the fact that the representation is accurate. The cache is certainly not populated on updates. So I think we're OK.

=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py	2010-03-24 17:37:26 +0000
+++ lib/canonical/database/sqlbase.py	2010-06-01 20:53:29 +0000
@@ -28,6 +28,8 @@
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
+from lazr.restful.interfaces import IRepresentationCache
+
 from canonical.config import config, dbconfig
 from canonical.database.interfaces import ISQLBase
 
@@ -246,6 +248,11 @@
         """Inverse of __eq__."""
         return not (self == other)
 
+    def __storm_flushed__(self):
+        """Invalidate the web service cache."""
+        cache = getUtility(IRepresentationCache)
+        cache.delete(self)
+
 alreadyInstalledMsg = ("A ZopelessTransactionManager with these settings is "
 "already installed.  This is probably caused by calling initZopeless twice.")
 

=== added file 'lib/canonical/launchpad/pagetests/webservice/cache.txt'
--- lib/canonical/launchpad/pagetests/webservice/cache.txt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/pagetests/webservice/cache.txt	2010-06-01 20:53:29 +0000
@@ -0,0 +1,55 @@
+************************
+The representation cache
+************************
+
+Launchpad stores JSON representations of objects in a memcached
+cache. The full cache functionality is tested in lazr.restful and in
+lib/lp/services/memcache/doc/restful-cache.txt. This is just a simple
+integration test.
+
+First we need to get a reference to the cache object, so we can look
+inside.
+
+    >>> from zope.component import getUtility
+    >>> from lazr.restful.interfaces import IRepresentationCache
+    >>> cache = getUtility(IRepresentationCache)
+
+Since the cache is keyed by the underlying database object, we also
+need one of those objects.
+
+    >>> from lp.registry.interfaces.person import IPersonSet
+    >>> login(ANONYMOUS)
+    >>> person = getUtility(IPersonSet).getByName('salgado')
+    >>> key = cache.key_for(person, 'application/json', 'devel')
+    >>> logout()
+
+The cache starts out empty.
+
+    >>> print cache.get_by_key(key)
+    None
+
+Retrieving a representation of an object populates the cache.
+
+    >>> ignore = webservice.get("/~salgado", api_version="devel").jsonBody()
+
+    >>> cache.get_by_key(key)
+    '{...}'
+
+Once the cache is populated with a representation, the cached
+representation is used in preference to generating a new
+representation of that object. We can verify this by putting a fake
+value into the cache and retrieving a representation of the
+corresponding object.
+
+    >>> import simplejson
+    >>> cache.set_by_key(key, simplejson.dumps("Fake representation"))
+
+    >>> print webservice.get("/~salgado", api_version="devel").jsonBody()
+    Fake representation
+
+Cleanup.
+
+    >>> cache.delete(person)
+
+    >>> webservice.get("/~salgado", api_version="devel").jsonBody()
+    {...}

=== modified file 'lib/canonical/launchpad/zcml/webservice.zcml'
--- lib/canonical/launchpad/zcml/webservice.zcml	2010-03-26 19:11:50 +0000
+++ lib/canonical/launchpad/zcml/webservice.zcml	2010-06-01 20:53:29 +0000
@@ -16,6 +16,11 @@
         provides="lazr.restful.interfaces.IWebServiceConfiguration">
     </utility>
 
+    <utility
+        factory="lp.services.memcache.restful.MemcachedStormRepresentationCache"
+        provides="lazr.restful.interfaces.IRepresentationCache">
+    </utility>
+
     <securedutility
         class="canonical.launchpad.systemhomes.WebServiceApplication"
         provides="canonical.launchpad.interfaces.IWebServiceApplication">

=== modified file 'lib/lp/services/memcache/client.py'
--- lib/lp/services/memcache/client.py	2009-09-16 12:47:23 +0000
+++ lib/lp/services/memcache/client.py	2010-06-01 20:53:29 +0000
@@ -4,7 +4,9 @@
 """Launchpad Memcache client."""
 
 __metaclass__ = type
-__all__ = []
+__all__ = [
+    'memcache_client_factory'
+    ]
 
 import memcache
 import re

=== added file 'lib/lp/services/memcache/doc/restful-cache.txt'
--- lib/lp/services/memcache/doc/restful-cache.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/services/memcache/doc/restful-cache.txt	2010-06-01 20:53:29 +0000
@@ -0,0 +1,102 @@
+***************************************
+The Storm/memcached representation cache
+****************************************
+
+The web service library lazr.restful will store the representations it
+generates in a cache, if a suitable cache implementation is
+provided. We implement a cache that stores representations of Storm
+objects in memcached.
+
+    >>> login('[e-mail]')
+
+    >>> from lp.services.memcache.restful import (
+    ...     MemcachedStormRepresentationCache)
+    >>> cache = MemcachedStormRepresentationCache()
+
+An object's cache key is derived from its Storm metadata: its database
+table name and its primary key.
+
+    >>> from zope.component import getUtility
+    >>> from lp.registry.interfaces.person import IPersonSet
+    >>> person = getUtility(IPersonSet).getByName('salgado')
+
+    >>> cache_key = cache.key_for(
+    ...     person, 'media/type', 'web-service-version')
+    >>> print person.id, cache_key
+    29 Person(29,),media/type,web-service-version
+
+    >>> from operator import attrgetter
+    >>> languages = sorted(person.languages, key=attrgetter('englishname'))
+    >>> for language in languages:
+    ...     cache_key = cache.key_for(
+    ...         language, 'media/type', 'web-service-version')
+    ...     print language.id, cache_key
+    119 Language(119,),media/type,web-service-version
+    521 Language(521,),media/type,web-service-version
+
+The cache starts out empty.
+
+    >>> json_type = 'application/json'
+
+    >>> print cache.get(person, json_type, "v1", default="missing")
+    missing
+
+Add a representation to the cache, and you can retrieve it later.
+
+    >>> cache.set(person, json_type, "beta",
+    ...           "This is a representation for version beta.")
+
+    >>> print cache.get(person, json_type, "beta")
+    This is a representation for version beta.
+
+A single object can cache different representations for different
+web service versions.
+
+    >>> cache.set(person, json_type, '1.0',
+    ...           'This is a different representation for version 1.0.')
+
+    >>> print cache.get(person, json_type, "1.0")
+    This is a different representation for version 1.0.
+
+The web service version doesn't have to actually be defined in the
+configuration. (But you shouldn't use this--see below!)
+
+    >>> cache.set(person, json_type, 'no-such-version',
+    ...           'This is a representation for a nonexistent version.')
+
+    >>> print cache.get(person, json_type, "no-such-version")
+    This is a representation for a nonexistent version.
+
+A single object can also cache different representations for different
+media types, not just application/json. (But you shouldn't use
+this--see below!)
+
+    >>> cache.set(person, 'media/type', '1.0',
+    ...           'This is a representation for a strange media type.')
+
+    >>> print cache.get(person, "media/type", "1.0")
+    This is a representation for a strange media type.
+
+When a Launchpad object is modified, its JSON representations for
+recognized web service versions are automatically removed from the
+cache.
+
+    >>> person.addressline1 = "New address"
+    >>> from canonical.launchpad.ftests import syncUpdate
+    >>> syncUpdate(person)
+
+    >>> print cache.get(person, json_type, "beta", default="missing")
+    missing
+
+    >>> print cache.get(person, json_type, "1.0", default="missing")
+    missing
+
+But non-JSON representations, and representations for unrecognized web
+service versions, are _not_ removed from the cache. (This is why you
+shouldn't put such representations into the cache.)
+
+    >>> print cache.get(person, json_type, "no-such-version")
+    This is a representation for a nonexistent version.
+
+    >>> print cache.get(person, "media/type", "1.0")
+    This is a representation for a strange media type.

=== added file 'lib/lp/services/memcache/restful.py'
--- lib/lp/services/memcache/restful.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/memcache/restful.py	2010-06-01 20:53:29 +0000
@@ -0,0 +1,39 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Storm/memcached implementation of lazr.restful's representation cache."""
+
+import storm
+
+from lp.services.memcache.client import memcache_client_factory
+from lazr.restful.simple import BaseRepresentationCache
+
+__metaclass__ = type
+__all__ = [
+    'MemcachedStormRepresentationCache',
+]
+
+
+class MemcachedStormRepresentationCache(BaseRepresentationCache):
+    """Caches lazr.restful representations of Storm objects in memcached."""
+
+    def __init__(self):
+        self.client = memcache_client_factory()
+
+    def key_for(self, obj, media_type, version):
+        storm_info = storm.info.get_obj_info(obj)
+        table_name = storm_info.cls_info.table
+        primary_key = tuple(var.get() for var in storm_info.primary_vars)
+
+        key = (table_name + repr(primary_key)
+                + ',' + media_type + ',' + str(version))
+        return key
+
+    def get_by_key(self, key, default=None):
+        return self.client.get(key) or default
+
+    def set_by_key(self, key, value):
+        self.client.set(key, value)
+
+    def delete_by_key(self, key):
+        self.client.delete(key)

=== modified file 'lib/lp/services/memcache/tests/test_doc.py'
--- lib/lp/services/memcache/tests/test_doc.py	2010-03-03 11:00:42 +0000
+++ lib/lp/services/memcache/tests/test_doc.py	2010-06-01 20:53:29 +0000
@@ -7,9 +7,7 @@
 
 import os.path
 from textwrap import dedent
-import unittest
 
-from zope.component import getUtility
 import zope.pagetemplate.engine
 from zope.pagetemplate.pagetemplate import PageTemplate
 from zope.publisher.browser import TestRequest
@@ -17,9 +15,7 @@
 from canonical.launchpad.testing.systemdocs import (
     LayeredDocFileSuite, setUp, tearDown)
 from canonical.testing.layers import LaunchpadFunctionalLayer, MemcachedLayer
-from lp.services.memcache.interfaces import IMemcacheClient
 from lp.services.testing import build_test_suite
-from lp.testing import TestCase
 
 
 here = os.path.dirname(os.path.realpath(__file__))
@@ -59,11 +55,15 @@
     test.globs['MemcachedLayer'] = MemcachedLayer
 
 
+def suite_for_doctest(filename):
+    return LayeredDocFileSuite(
+        '../doc/%s' % filename,
+        setUp=memcacheSetUp, tearDown=tearDown,
+        layer=LaunchpadFunctionalLayer)
+
 special = {
-    'tales-cache.txt': LayeredDocFileSuite(
-        '../doc/tales-cache.txt',
-        setUp=memcacheSetUp, tearDown=tearDown,
-        layer=LaunchpadFunctionalLayer),
+    'tales-cache.txt': suite_for_doctest('tales-cache.txt'),
+    'restful-cache.txt': suite_for_doctest('restful-cache.txt'),
     }
 
 

=== modified file 'versions.cfg'
--- versions.cfg	2010-05-17 20:03:02 +0000
+++ versions.cfg	2010-06-01 20:53:29 +0000
@@ -28,7 +28,7 @@
 lazr.delegates = 1.1.0
 lazr.enum = 1.1.2
 lazr.lifecycle = 1.1
-lazr.restful = 0.9.26
+lazr.restful = 0.9.27
 lazr.restfulclient = 0.9.14
 lazr.smtptest = 1.1
 lazr.testing = 0.1.1