Launchpad itself

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

  1. 616331-private-builds-in-builder-history
  2. Merge into db-devel
Proposed by Michael Nelson
Status: Merged
Approved by: Edwin Grubbs
Approved revision: no longer in the source branch.
Merged at revision: 9650
Proposed branch: lp:~michael.nelson/launchpad/616331-private-builds-in-builder-history
Merge into: lp:launchpad/db-devel
Diff against target: 82 lines (+21/-32)
1 file modified
lib/lp/buildmaster/model/buildfarmjob.py (+21/-32)
To merge this branch: bzr merge lp:~michael.nelson/launchpad/616331-private-builds-in-builder-history
Reviewer Review Type Date Requested Status
Julian Edwards (community) release-critical Approve
Edwin Grubbs (community) code Approve
Launchpad code reviewers Pending
Review via email: mp+32355@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

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)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

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!

review_edwin.diff
1=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
2--- lib/lp/buildmaster/model/buildfarmjob.py 2010-08-11 17:09:45 +0000
3+++ lib/lp/buildmaster/model/buildfarmjob.py 2010-08-11 21:26:07 +0000
4@@ -365,15 +365,18 @@
5 # Currently only package builds can be private (via their
6 # related archive), but not all build farm jobs will have a
7 # related package build - hence the left join.
8- left_join_pkg_builds = LeftJoin(
9- PackageBuild, PackageBuild.build_farm_job == BuildFarmJob.id)
10- left_join_archive = LeftJoin(
11- Archive, PackageBuild.archive == Archive.id)
12- left_join_participation = LeftJoin(
13- TeamParticipation, TeamParticipation.teamID == Archive.ownerID)
14+ origin = [BuildFarmJob]
15+ left_join_archive = [
16+ LeftJoin(
17+ PackageBuild,
18+ PackageBuild.build_farm_job == BuildFarmJob.id),
19+ LeftJoin(
20+ Archive, PackageBuild.archive == Archive.id),
21+ ]
22
23 if user is None:
24 # Anonymous requests don't get to see private builds at all.
25+ origin.extend(left_join_archive)
26 extra_clauses.append(Coalesce(Archive.private, False) == False)
27
28 elif user.inTeam(getUtility(ILaunchpadCelebrities).admin):
29@@ -382,16 +385,15 @@
30 else:
31 # Everyone else sees all public builds and the
32 # specific private builds to which they have access.
33+ origin.extend(left_join_archive)
34+ origin.append(LeftJoin(
35+ TeamParticipation, TeamParticipation.teamID == Archive.ownerID))
36 extra_clauses.append(
37 Or(Coalesce(Archive.private, False) == False,
38 TeamParticipation.person == user))
39
40- filtered_builds = IStore(BuildFarmJob).using(
41- BuildFarmJob,
42- left_join_pkg_builds,
43- left_join_archive,
44- left_join_participation,
45- ).find(BuildFarmJob, *extra_clauses)
46+ filtered_builds = IStore(BuildFarmJob).using(*origin).find(
47+ BuildFarmJob, *extra_clauses)
48
49 filtered_builds.order_by(
50 Desc(BuildFarmJob.date_finished), BuildFarmJob.id)
Revision history for this message
Julian Edwards (julian-edwards) :
review: Approve (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
2--- lib/lp/buildmaster/model/buildfarmjob.py 2010-06-16 16:01:08 +0000
3+++ lib/lp/buildmaster/model/buildfarmjob.py 2010-08-11 21:29:47 +0000
4@@ -16,7 +16,7 @@
5
6 import pytz
7
8-from storm.expr import And, Coalesce, Desc, Join, LeftJoin, Select
9+from storm.expr import Coalesce, Desc, LeftJoin, Or
10 from storm.locals import Bool, DateTime, Int, Reference, Storm
11 from storm.store import Store
12
13@@ -365,49 +365,38 @@
14 # Currently only package builds can be private (via their
15 # related archive), but not all build farm jobs will have a
16 # related package build - hence the left join.
17- left_join_pkg_builds = LeftJoin(
18- BuildFarmJob,
19- Join(
20+ origin = [BuildFarmJob]
21+ left_join_archive = [
22+ LeftJoin(
23 PackageBuild,
24- Archive,
25- And(PackageBuild.archive == Archive.id)),
26- PackageBuild.build_farm_job == BuildFarmJob.id)
27-
28- filtered_builds = IStore(BuildFarmJob).using(
29- left_join_pkg_builds).find(BuildFarmJob, *extra_clauses)
30+ PackageBuild.build_farm_job == BuildFarmJob.id),
31+ LeftJoin(
32+ Archive, PackageBuild.archive == Archive.id),
33+ ]
34
35 if user is None:
36 # Anonymous requests don't get to see private builds at all.
37- filtered_builds = filtered_builds.find(
38- Coalesce(Archive.private, False) == False)
39+ origin.extend(left_join_archive)
40+ extra_clauses.append(Coalesce(Archive.private, False) == False)
41
42 elif user.inTeam(getUtility(ILaunchpadCelebrities).admin):
43 # Admins get to see everything.
44 pass
45 else:
46- # Everyone else sees a union of all public builds and the
47+ # Everyone else sees all public builds and the
48 # specific private builds to which they have access.
49- filtered_builds = filtered_builds.find(
50- Coalesce(Archive.private, False) == False)
51-
52- user_teams_subselect = Select(
53- TeamParticipation.teamID,
54- where=And(
55- TeamParticipation.personID == user.id,
56- TeamParticipation.teamID == Archive.ownerID))
57- private_builds_for_user = IStore(BuildFarmJob).find(
58- BuildFarmJob,
59- PackageBuild.build_farm_job == BuildFarmJob.id,
60- PackageBuild.archive == Archive.id,
61- Archive.private == True,
62- Archive.ownerID.is_in(user_teams_subselect),
63- *extra_clauses)
64-
65- filtered_builds = filtered_builds.union(
66- private_builds_for_user)
67+ origin.extend(left_join_archive)
68+ origin.append(LeftJoin(
69+ TeamParticipation, TeamParticipation.teamID == Archive.ownerID))
70+ extra_clauses.append(
71+ Or(Coalesce(Archive.private, False) == False,
72+ TeamParticipation.person == user))
73+
74+ filtered_builds = IStore(BuildFarmJob).using(*origin).find(
75+ BuildFarmJob, *extra_clauses)
76
77 filtered_builds.order_by(
78 Desc(BuildFarmJob.date_finished), BuildFarmJob.id)
79+ filtered_builds.config(distinct=True)
80
81 return filtered_builds
82-

Subscribers

People subscribed via source and target branches

to status/vote changes: