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
=== modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
--- lib/lp/buildmaster/model/buildfarmjob.py 2010-08-11 17:09:45 +0000
+++ lib/lp/buildmaster/model/buildfarmjob.py 2010-08-11 21:26:07 +0000
@@ -365,15 +365,18 @@
365 # Currently only package builds can be private (via their365 # Currently only package builds can be private (via their
366 # related archive), but not all build farm jobs will have a366 # related archive), but not all build farm jobs will have a
367 # related package build - hence the left join.367 # related package build - hence the left join.
368 left_join_pkg_builds = LeftJoin(368 origin = [BuildFarmJob]
369 PackageBuild, PackageBuild.build_farm_job == BuildFarmJob.id)369 left_join_archive = [
370 left_join_archive = LeftJoin(370 LeftJoin(
371 Archive, PackageBuild.archive == Archive.id)371 PackageBuild,
372 left_join_participation = LeftJoin(372 PackageBuild.build_farm_job == BuildFarmJob.id),
373 TeamParticipation, TeamParticipation.teamID == Archive.ownerID)373 LeftJoin(
374 Archive, PackageBuild.archive == Archive.id),
375 ]
374376
375 if user is None:377 if user is None:
376 # Anonymous requests don't get to see private builds at all.378 # Anonymous requests don't get to see private builds at all.
379 origin.extend(left_join_archive)
377 extra_clauses.append(Coalesce(Archive.private, False) == False)380 extra_clauses.append(Coalesce(Archive.private, False) == False)
378381
379 elif user.inTeam(getUtility(ILaunchpadCelebrities).admin):382 elif user.inTeam(getUtility(ILaunchpadCelebrities).admin):
@@ -382,16 +385,15 @@
382 else:385 else:
383 # Everyone else sees all public builds and the386 # Everyone else sees all public builds and the
384 # specific private builds to which they have access.387 # specific private builds to which they have access.
388 origin.extend(left_join_archive)
389 origin.append(LeftJoin(
390 TeamParticipation, TeamParticipation.teamID == Archive.ownerID))
385 extra_clauses.append(391 extra_clauses.append(
386 Or(Coalesce(Archive.private, False) == False,392 Or(Coalesce(Archive.private, False) == False,
387 TeamParticipation.person == user))393 TeamParticipation.person == user))
388394
389 filtered_builds = IStore(BuildFarmJob).using(395 filtered_builds = IStore(BuildFarmJob).using(*origin).find(
390 BuildFarmJob,396 BuildFarmJob, *extra_clauses)
391 left_join_pkg_builds,
392 left_join_archive,
393 left_join_participation,
394 ).find(BuildFarmJob, *extra_clauses)
395397
396 filtered_builds.order_by(398 filtered_builds.order_by(
397 Desc(BuildFarmJob.date_finished), BuildFarmJob.id)399 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
=== 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 @@
1616
17import pytz17import pytz
1818
19from storm.expr import And, Coalesce, Desc, Join, LeftJoin, Select19from storm.expr import Coalesce, Desc, LeftJoin, Or
20from storm.locals import Bool, DateTime, Int, Reference, Storm20from storm.locals import Bool, DateTime, Int, Reference, Storm
21from storm.store import Store21from storm.store import Store
2222
@@ -365,49 +365,38 @@
365 # Currently only package builds can be private (via their365 # Currently only package builds can be private (via their
366 # related archive), but not all build farm jobs will have a366 # related archive), but not all build farm jobs will have a
367 # related package build - hence the left join.367 # related package build - hence the left join.
368 left_join_pkg_builds = LeftJoin(368 origin = [BuildFarmJob]
369 BuildFarmJob,369 left_join_archive = [
370 Join(370 LeftJoin(
371 PackageBuild,371 PackageBuild,
372 Archive,372 PackageBuild.build_farm_job == BuildFarmJob.id),
373 And(PackageBuild.archive == Archive.id)),373 LeftJoin(
374 PackageBuild.build_farm_job == BuildFarmJob.id)374 Archive, PackageBuild.archive == Archive.id),
375375 ]
376 filtered_builds = IStore(BuildFarmJob).using(
377 left_join_pkg_builds).find(BuildFarmJob, *extra_clauses)
378376
379 if user is None:377 if user is None:
380 # Anonymous requests don't get to see private builds at all.378 # Anonymous requests don't get to see private builds at all.
381 filtered_builds = filtered_builds.find(379 origin.extend(left_join_archive)
382 Coalesce(Archive.private, False) == False)380 extra_clauses.append(Coalesce(Archive.private, False) == False)
383381
384 elif user.inTeam(getUtility(ILaunchpadCelebrities).admin):382 elif user.inTeam(getUtility(ILaunchpadCelebrities).admin):
385 # Admins get to see everything.383 # Admins get to see everything.
386 pass384 pass
387 else:385 else:
388 # Everyone else sees a union of all public builds and the386 # Everyone else sees all public builds and the
389 # specific private builds to which they have access.387 # specific private builds to which they have access.
390 filtered_builds = filtered_builds.find(388 origin.extend(left_join_archive)
391 Coalesce(Archive.private, False) == False)389 origin.append(LeftJoin(
392390 TeamParticipation, TeamParticipation.teamID == Archive.ownerID))
393 user_teams_subselect = Select(391 extra_clauses.append(
394 TeamParticipation.teamID,392 Or(Coalesce(Archive.private, False) == False,
395 where=And(393 TeamParticipation.person == user))
396 TeamParticipation.personID == user.id,394
397 TeamParticipation.teamID == Archive.ownerID))395 filtered_builds = IStore(BuildFarmJob).using(*origin).find(
398 private_builds_for_user = IStore(BuildFarmJob).find(396 BuildFarmJob, *extra_clauses)
399 BuildFarmJob,
400 PackageBuild.build_farm_job == BuildFarmJob.id,
401 PackageBuild.archive == Archive.id,
402 Archive.private == True,
403 Archive.ownerID.is_in(user_teams_subselect),
404 *extra_clauses)
405
406 filtered_builds = filtered_builds.union(
407 private_builds_for_user)
408397
409 filtered_builds.order_by(398 filtered_builds.order_by(
410 Desc(BuildFarmJob.date_finished), BuildFarmJob.id)399 Desc(BuildFarmJob.date_finished), BuildFarmJob.id)
400 filtered_builds.config(distinct=True)
411401
412 return filtered_builds402 return filtered_builds
413

Subscribers

People subscribed via source and target branches

to status/vote changes: