Merge lp:~michael.nelson/launchpad/616331-private-builds-in-builder-history into lp:launchpad/db-devel

[Michael Nelson]

Fix timeout on builder history page by replacing nested joins in query

Overview
========
We started seeing timeouts on dogfood (and apparently staging) recently for the builder history page. This branch fixes those timeouts.

Details
=======
Pre-implementation discussion (and help from stub to fix the query) can be seen at bug 616331.

The branch replaces the nested joins with normal joins. I was not able to get rid of the COALESCE.

To test:
========
bin/test -vvm test_buildfarmjob -t TestBuildFarmJobSet

I've also merged this branch on dogfood so that the page now displays fine at:

https://dogfood.launchpad.net/builders/rubidium/+history

No lint.

As per the discussion on the bug, this will either be included in the re-roll or a CP.

[Edwin Grubbs (edwin-grubbs)]

Hi Michael,

This is a nice improvement. Since the TeamParticipation table won't be used if the user is anonymous or an admin, it would be a small but easy optimization to not join it into those queries. For example:

origin = [
  BuildFarmJob,
  LeftJoin(PackageBuild, ...),
  LeftJoin(Archive, ...),
  ]

if ANONYMOUS:
  ...
elif ADMIN:
  ...
else:
  origin.append(LeftJoin(TeamParticipation, ...))

filtered_builds = IStore(BuildFarmJob).using(*origin).find(...)

I guess if an admin is running the query, there is no reason to use any tables besides BuildFarmJob.

-Edwin

review: Approve (code)

[Michael Nelson (michael.nelson)]

On Wed, Aug 11, 2010 at 7:54 PM, Edwin Grubbs
<email address hidden> wrote:
> Review: Approve code
> Hi Michael,
>
> This is a nice improvement. Since the TeamParticipation table won't be used if the user is anonymous or an admin, it would be a small but easy optimization to not join it into those queries. For example:
>
> origin = [
>  BuildFarmJob,
>  LeftJoin(PackageBuild, ...),
>  LeftJoin(Archive, ...),
>  ]
>
> if ANONYMOUS:
>  ...
> elif ADMIN:
>  ...
> else:
>  origin.append(LeftJoin(TeamParticipation, ...))
>
> filtered_builds = IStore(BuildFarmJob).using(*origin).find(...)
>
> I guess if an admin is running the query, there is no reason to use any tables besides BuildFarmJob.

Yes... good point. I've attached an incremental (pushed as r9651) that
only uses the joins when necessary. Thanks!

[Julian Edwards (julian-edwards)]

review: Approve (release-critical)

=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py	2010-06-16 16:01:08 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py	2010-08-11 21:29:47 +0000
@@ -16,7 +16,7 @@
 
 import pytz
 
-from storm.expr import And, Coalesce, Desc, Join, LeftJoin, Select
+from storm.expr import Coalesce, Desc, LeftJoin, Or
 from storm.locals import Bool, DateTime, Int, Reference, Storm
 from storm.store import Store
 
@@ -365,49 +365,38 @@
         # Currently only package builds can be private (via their
         # related archive), but not all build farm jobs will have a
         # related package build - hence the left join.
-        left_join_pkg_builds = LeftJoin(
-            BuildFarmJob,
-            Join(
+        origin = [BuildFarmJob]
+        left_join_archive = [
+            LeftJoin(
                 PackageBuild,
-                Archive,
-                And(PackageBuild.archive == Archive.id)),
-            PackageBuild.build_farm_job == BuildFarmJob.id)
-
-        filtered_builds = IStore(BuildFarmJob).using(
-            left_join_pkg_builds).find(BuildFarmJob, *extra_clauses)
+                PackageBuild.build_farm_job == BuildFarmJob.id),
+            LeftJoin(
+                Archive, PackageBuild.archive == Archive.id),
+            ]
 
         if user is None:
             # Anonymous requests don't get to see private builds at all.
-            filtered_builds = filtered_builds.find(
-                Coalesce(Archive.private, False) == False)
+            origin.extend(left_join_archive)
+            extra_clauses.append(Coalesce(Archive.private, False) == False)
 
         elif user.inTeam(getUtility(ILaunchpadCelebrities).admin):
             # Admins get to see everything.
             pass
         else:
-            # Everyone else sees a union of all public builds and the
+            # Everyone else sees all public builds and the
             # specific private builds to which they have access.
-            filtered_builds = filtered_builds.find(
-                Coalesce(Archive.private, False) == False)
-
-            user_teams_subselect = Select(
-                TeamParticipation.teamID,
-                where=And(
-                   TeamParticipation.personID == user.id,
-                   TeamParticipation.teamID == Archive.ownerID))
-            private_builds_for_user = IStore(BuildFarmJob).find(
-                BuildFarmJob,
-                PackageBuild.build_farm_job == BuildFarmJob.id,
-                PackageBuild.archive == Archive.id,
-                Archive.private == True,
-                Archive.ownerID.is_in(user_teams_subselect),
-                *extra_clauses)
-
-            filtered_builds = filtered_builds.union(
-                private_builds_for_user)
+            origin.extend(left_join_archive)
+            origin.append(LeftJoin(
+                TeamParticipation, TeamParticipation.teamID == Archive.ownerID))
+            extra_clauses.append(
+                Or(Coalesce(Archive.private, False) == False,
+                   TeamParticipation.person == user))
+
+        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
-