Code review comment for lp:~leonardr/launchpad/test-representation-cache

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 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 up with two clients and twice as many sockets opened to each
>> memcached. Or we need to refactor the factory to return a singleton
>> (is that still a factory?)
>
> I didn't know about getUtility(IMemcacheClient)--I was copying code from
> somewhere else. I'll fix it.

I think it should be fine as long as the IMemcacheClient zcml utility
directive is processed before the new one.

>> > +    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)
>>
>>
>> We will likely need to calculate the key using the Storm class and the
>> primary key value rather than an actual object instance. No need to
>> add it now, but it might affect your API design.
>
> Can you explain why? Is this related to the database hook? When I
> _populate_ the cache, I have an object instance--I guess I can use
> storm_info to get the Storm class and the primary key value. Since
> lazr.restful never calls the delete code itself, we can implement it
> however we want.

Just thinking about scripts doing bulk updates using raw sql or
similar. We won't need it if we can do the invalidation at the
database level.

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/