launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

I haven't read the diff here in whole, but I'd like to note that this appears as though it will trigger late evaluation - the display_subscribed_by property isn't eager loaded, and a bug with a couple hundred subscriptions will cause eager loading of a couple hundred person objects *unless* they are all self-subscribed. It will also be a little slow because its comparing objects not keys - the property does:
        if self.person == self.subscribed_by:
            return u'Self-subscribed'
        else:
            return u'Subscribed by %s' % self.subscribed_by.displayname

This would be a little more efficient as

       if self.person_id == self.subscribed_by_id:
            return u'Self-subscribed'
        else:
            return u'Subscribed by %s' % self.subscribed_by.displayname

but the big thing is eager loading: we need to populate the subscribing person (but only for views that need it) en masse, rather than just-in-time.

I don't know if this will cause timeouts, but this sort of thing is one of the key drivers for timeouts.
-- 
https://code.launchpad.net/~bac/launchpad/bug-799901/+merge/68590
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-799901 into lp:launchpad.