Launchpad itself

Merge lp:~lifeless/launchpad/registry into lp:launchpad

  1. registry
  2. Merge into devel
Proposed by Robert Collins
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: 11389
Proposed branch: lp:~lifeless/launchpad/registry
Merge into: lp:launchpad
Diff against target: 89 lines (+26/-9)
1 file modified
lib/lp/registry/model/person.py (+26/-9)
To merge this branch: bzr merge lp:~lifeless/launchpad/registry
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+32953@code.launchpad.net

Commit message

Drop the use of ValidPersonCache from Person._all_members as we usually link EmailAddress in anyway so we can get a more efficient query this way. (6000ms->35ms) for ubuntu-dev

Description of the change

So the ValidPersonCache component turns out to turn a 35ms thing into a 6000ms thing. Not so good: this unpacks the view, and performs snappily, fixing timeouts for this on staging.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Two changes discussed on IRC:
 * Indentation—don't mix "right after (" with "tab-indented on following lines."
 * AND should be And.

With that, you have my blessing.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/model/person.py'
2--- lib/lp/registry/model/person.py 2010-08-17 13:53:04 +0000
3+++ lib/lp/registry/model/person.py 2010-08-18 05:26:48 +0000
4@@ -190,6 +190,12 @@
5 """Flags if a Person is active and usable in Launchpad.
6
7 This is readonly, as this is a view in the database.
8+
9+ Note that it performs poorly at least some of the time, and if
10+ EmailAddress and Person are already being queried, its probably better to
11+ query Account directly. See bug
12+ https://bugs.edge.launchpad.net/launchpad-registry/+bug/615237 for some
13+ corroborating information.
14 """
15
16
17@@ -1489,7 +1495,7 @@
18 columns.append(KarmaTotalCache)
19 if need_ubuntu_coc:
20 columns.append(Alias(Exists(Select(SignedCodeOfConduct,
21- AND(Person._is_ubuntu_coc_signer_condition(),
22+ And(Person._is_ubuntu_coc_signer_condition(),
23 SignedCodeOfConduct.ownerID == Person.id))),
24 name='is_ubuntu_coc_signer'))
25 if need_location:
26@@ -1509,8 +1515,9 @@
27 Archive.id == Select(Min(Archive.id),
28 Archive.owner == Person.id, Archive),
29 Archive.purpose == ArchivePurpose.PPA)))
30- if need_preferred_email:
31- # Teams don't have email.
32+ # checking validity requires having a preferred email.
33+ if need_preferred_email or need_validity:
34+ # Teams don't have email, so a left join
35 origin.append(
36 LeftJoin(EmailAddress, EmailAddress.person == Person.id))
37 columns.append(EmailAddress)
38@@ -1518,10 +1525,14 @@
39 Or(EmailAddress.status == None,
40 EmailAddress.status == EmailAddressStatus.PREFERRED))
41 if need_validity:
42- # May find invalid persons
43+ # May find teams (teams are not valid people)
44 origin.append(
45- LeftJoin(ValidPersonCache, ValidPersonCache.id == Person.id))
46- columns.append(ValidPersonCache)
47+ LeftJoin(Account, Person.account == Account.id))
48+ columns.append(Account)
49+ conditions = And(conditions,
50+ Or(
51+ Account.status == None,
52+ Account.status == AccountStatus.ACTIVE))
53 if len(columns) == 1:
54 columns = columns[0]
55 # Return a simple ResultSet
56@@ -1562,8 +1573,14 @@
57 email = row[index]
58 index += 1
59 cache_property(result, '_preferredemail_cached', email)
60+ #-- validity caching
61 if need_validity:
62- valid = row[index] is not None
63+ # valid if:
64+ valid = (
65+ # -- valid account found
66+ row[index] is not None
67+ # -- preferred email found
68+ and result.preferredemail is not None)
69 index += 1
70 cache_property(result, '_is_valid_person_cached', valid)
71 return result
72@@ -2409,7 +2426,7 @@
73 """See `IPerson`."""
74 # Also assigned to by self._all_members.
75 store = Store.of(self)
76- query = AND(SignedCodeOfConduct.ownerID == self.id,
77+ query = And(SignedCodeOfConduct.ownerID == self.id,
78 Person._is_ubuntu_coc_signer_condition())
79 # TODO: Using exists would be faster than count().
80 return bool(store.find(SignedCodeOfConduct, query).count())
81@@ -2419,7 +2436,7 @@
82 """Generate a Storm Expr for determing the coc signing status."""
83 sigset = getUtility(ISignedCodeOfConductSet)
84 lastdate = sigset.getLastAcceptedDate()
85- return AND(SignedCodeOfConduct.active == True,
86+ return And(SignedCodeOfConduct.active == True,
87 SignedCodeOfConduct.datecreated >= lastdate)
88
89 @property