Code review comment for lp:~mwhudson/launchpad/much-faster-rewrite-map

Hi Stuart, thanks for taking a look!

Stuart Bishop wrote:
> On Wed, Jul 22, 2009 at 11:27 AM, Tim Penhey<email address hidden> wrote:
>>> + self.store = getUtility(IStoreSelector).get(MAIN_STORE,
>> SLAVE_FLAVOR)
>
> You can spell this 'self.store = ISlaveStore(Branch)' which I think is
> nicer and more meaningful.

Ah, that is nicer.

>>> + if prefix in self._cache:
>>> + branch_id, inserted_time = self._cache[prefix]
>>> + if (self._now() < inserted_time +
>>> + config.codehosting.branch_rewrite_cache_lifetime):
>>> + trailing = location[len(prefix) + 1:]
>>> + return branch_id, trailing, "HIT"
>>> + prefixes.append(prefix)
>>> + result = self.store.find(
>>> + Branch,
>>> + Branch.unique_name.is_in(prefixes), Branch.private == False)
>>> + try:
>>> + branch_id, unique_name = result.values(
>>> + Branch.id, Branch.unique_name).next()
>>> + except StopIteration:
>>> + return None, None, "MISS"
>
> How do we know the Branch was retrieved from the database and not the
> Storm cache?

We don't. Is there some way I could test for this?

> I also don't see any commits. Both of these issues can be fixed in the
> schema-lazr.conf - set the isolation level to autocommit and the
> storm_cache_size to 0 for this component.

Why is autocommit better than read committed, given that we don't write
to the database? Serializable would be very very wrong....

I also admit that I don't know what I have to change in schema-lazr.conf
to change the cache size -- what do you mean by 'component'?

>> How about:
>>
>> result = self.store.find(
>> (Branch.id, Branch.unique_name),
>> Branch.unique_name.is_in(prefixes), Branch.private == False).one()
>>
>> That way result should be None if not found, and a tuple of id and unique_name
>> if it was found.
>>
>> That'll make this code a little cleaner.
>
> Shouldn't that be .any()? .one() will explode if multiple rows matched.

We want it to explode if multiple rows match.

Cheers,
mwh