launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/builder-history-better-query into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1001838 in Launchpad itself: "Builder:+history timeouts due to terrible main query"
  https://bugs.launchpad.net/launchpad/+bug/1001838

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/builder-history-better-query/+merge/123676

Rewrite the two (seperate) queries that are constructed by IBuildFarmJobSet.getBuildsForBuilder(). This, in conjunction with https://code.launchpad.net/~stevenk/launchpad/buildfarmjob-index/+merge/123675 pulls down the major query used on Builder:+history from 9 seconds to about 2 ms. For a further win, it reduces the line count of the function by a fair bit.
-- 
https://code.launchpad.net/~stevenk/launchpad/builder-history-better-query/+merge/123676
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/builder-history-better-query into lp:launchpad.

=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py	2012-01-02 11:21:14 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py	2012-09-11 04:40:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -9,18 +9,16 @@
     'BuildFarmJobOldDerived',
     ]
 
-
 import hashlib
 
 from lazr.delegates import delegates
 import pytz
 from storm.expr import (
-    And,
     Desc,
     LeftJoin,
+    Not,
     Or,
     Select,
-    Union,
     )
 from storm.locals import (
     Bool,
@@ -42,7 +40,6 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import (
     BuildFarmJobType,
     BuildStatus,
@@ -56,6 +53,7 @@
     ISpecificBuildFarmJobSource,
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.enumcol import DBEnum
@@ -437,70 +435,28 @@
                 PackageBuild.build_farm_job == BuildFarmJob.id),
             ]
 
-        # STORM syntax has totally obfuscated this query and wasted
-        # THREE hours of my time converting perfectly good SQL syntax.  I'm
-        # really sorry if you're the poor sap who has to maintain this.
-
-        inner_privacy_query = (
-            Union(
-                Select(
-                    Archive.id,
-                    tables=(Archive,),
-                    where=(Archive._private == False)
-                    ),
-                Select(
-                    Archive.id,
-                    tables=(Archive,),
-                    where=And(
-                        Archive._private == True,
-                        Archive.ownerID.is_in(
-                            Select(
-                                TeamParticipation.teamID,
-                                where=(TeamParticipation.person == user),
-                                distinct=True
-                            )
-                        )
-                    )
-                )
-            )
-        )
-
         if user is None:
             # Anonymous requests don't get to see private builds at all.
-            extra_clauses.append(
-                Or(
-                    PackageBuild.id == None,
-                    PackageBuild.archive_id.is_in(
+            origin.append(
+                LeftJoin(Archive, Archive.id == PackageBuild.archive_id))
+            extra_clauses.append(
+                Or(PackageBuild.id == None, Not(Archive._private)))
+        elif not IPersonRoles(user).in_admin:
+            # Non-admin users see all public builds and the specific
+            # private builds to which they have access.
+            origin.append(
+                LeftJoin(Archive, Archive.id == PackageBuild.archive_id))
+            extra_clauses.append(
+                Or(PackageBuild.id == None,
+                    Not(Archive._private), Archive.ownerID.is_in(
                         Select(
-                            Archive.id,
-                            tables=(Archive,),
-                            where=(Archive._private == False)
-                            )
-                        )
-                    )
-                )
-
-        elif user.inTeam(getUtility(ILaunchpadCelebrities).admin):
-            # Admins get to see everything.
-            pass
-        else:
-            # Everyone else sees all public builds and the
-            # specific private builds to which they have access.
-            extra_clauses.append(
-                Or(
-                    PackageBuild.id == None,
-                    PackageBuild.archive_id.is_in(inner_privacy_query)
-                    )
-                )
-
-        filtered_builds = IStore(BuildFarmJob).using(*origin).find(
-            BuildFarmJob, *extra_clauses)
-
-        filtered_builds.order_by(
-            Desc(BuildFarmJob.date_finished), BuildFarmJob.id)
-        filtered_builds.config(distinct=True)
-
-        return filtered_builds
+                            TeamParticipation.teamID,
+                            where=(TeamParticipation.person == user),
+                            distinct=True))))
+
+        return IStore(BuildFarmJob).using(*origin).find(
+            BuildFarmJob, *extra_clauses).order_by(
+                Desc(BuildFarmJob.date_finished), BuildFarmJob.id)
 
     def getByID(self, job_id):
         """See `IBuildfarmJobSet`."""