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

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.

>> +            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?
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.

> 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.

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