launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-590708-getBuildsByArchIds-timeout into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #590708 DistroSeries.getBuildRecords often timing out
  https://bugs.launchpad.net/bugs/590708


This branch fixes getBuildRecords API timeouts (bug #590708) by working around a really bad query plan.

In the current code, the query produced by getBuildsByArchIds takes roughly 6 seconds to run (see the last analysis on http://pastebin.com/AT04PS1P). This turns out to be because of the join against Archive, which somehow throws the planner's cost estimates off. Replacing it with a basic equivalent subquery does not help significantly, but an unnecessarily complicated one speeds it up massively (https://bugs.edge.launchpad.net/soyuz/+bug/590708/comments/8). There's further discussion in the bug which I shall not repeat here.

Rather than taking that route, however, I opted to use a solution employed frequently elsewhere in Soyuz: precalculating the archive IDs from the distribution, and passing them into the query directly. This takes it down to ~280ms on staging, and ~170ms on production (http://paste.ubuntu.com/475813/).
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-590708-getBuildsByArchIds-timeout/+merge/32181
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-590708-getBuildsByArchIds-timeout into lp:launchpad/devel.

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2010-08-03 14:59:22 +0000
+++ lib/lp/registry/model/distribution.py	2010-08-10 11:01:15 +0000
@@ -817,7 +817,7 @@
         # Use the facility provided by IBinaryPackageBuildSet to
         # retrieve the records.
         return getUtility(IBinaryPackageBuildSet).getBuildsByArchIds(
-            arch_ids, build_state, name, pocket)
+            self, arch_ids, build_state, name, pocket)
 
     def getSourcePackageCaches(self, archive=None):
         """See `IDistribution`."""

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-08-10 02:56:06 +0000
+++ lib/lp/registry/model/distroseries.py	2010-08-10 11:01:15 +0000
@@ -1130,7 +1130,7 @@
         # Use the facility provided by IBinaryPackageBuildSet to
         # retrieve the records.
         return getUtility(IBinaryPackageBuildSet).getBuildsByArchIds(
-            arch_ids, build_state, name, pocket)
+            self.distribution, arch_ids, build_state, name, pocket)
 
     def createUploadedSourcePackageRelease(
         self, sourcepackagename, version, maintainer, builddepends,

=== modified file 'lib/lp/soyuz/doc/binarypackagebuild.txt'
--- lib/lp/soyuz/doc/binarypackagebuild.txt	2010-07-17 12:56:41 +0000
+++ lib/lp/soyuz/doc/binarypackagebuild.txt	2010-08-10 11:01:15 +0000
@@ -422,33 +422,33 @@
 
     >>> hoary = ubuntu.getSeries('hoary')
     >>> arch_ids = [arch.id for arch in hoary.architectures]
-    >>> bs.getBuildsByArchIds(arch_ids).count()
+    >>> bs.getBuildsByArchIds(ubuntu, arch_ids).count()
     5
 
 It still working for empty list or None:
 
-    >>> bs.getBuildsByArchIds([]).count()
+    >>> bs.getBuildsByArchIds(ubuntu, []).count()
     0
 
-    >>> bs.getBuildsByArchIds(None).count()
+    >>> bs.getBuildsByArchIds(ubuntu, None).count()
     0
 
 Using build status, only the successfully built ones:
 
-    >>> bs.getBuildsByArchIds(arch_ids,
+    >>> bs.getBuildsByArchIds(ubuntu, arch_ids,
     ...     status=BuildStatus.FULLYBUILT).count()
     2
 
 Check the result content:
 
-    >>> [b.title for b in bs.getBuildsByArchIds(arch_ids,
+    >>> [b.title for b in bs.getBuildsByArchIds(ubuntu, arch_ids,
     ...     status=BuildStatus.FULLYBUILT)]
     [u'i386 build of pmount 0.1-1 in ubuntu hoary RELEASE', u'hppa build
     of pmount 0.1-1 in ubuntu hoary RELEASE']
 
 Using optional 'name' filter (matching with SQL LIKE %||filter||%)
 
-    >>> bs.getBuildsByArchIds(arch_ids,
+    >>> bs.getBuildsByArchIds(ubuntu, arch_ids,
     ...     status=BuildStatus.FULLYBUILT,
     ...     name='pmo').count()
     2
@@ -456,11 +456,11 @@
 Checking optional 'pocket' restriction:
 
     >>> from lp.registry.interfaces.pocket import PackagePublishingPocket
-    >>> bs.getBuildsByArchIds(arch_ids,
+    >>> bs.getBuildsByArchIds(ubuntu, arch_ids,
     ...     pocket=PackagePublishingPocket.UPDATES).count()
     0
 
-    >>> bs.getBuildsByArchIds(arch_ids,
+    >>> bs.getBuildsByArchIds(ubuntu, arch_ids,
     ...     pocket=PackagePublishingPocket.RELEASE).count()
     5
 
@@ -470,7 +470,7 @@
     >>> breezy = ubuntu.getSeries('breezy-autotest')
     >>> arch_ids = [arch.id for arch in breezy.architectures]
     >>> [(build.archive.purpose.name, build.title) for build in
-    ...    bs.getBuildsByArchIds(arch_ids, name='commercialpackage')]
+    ...    bs.getBuildsByArchIds(ubuntu, arch_ids, name='commercialpackage')]
     [('PARTNER', u'i386 build of commercialpackage 1.0-1 in ubuntu breezy-autotest RELEASE')]
 
 `IBinaryPackageBuildSet` also provides getStatusSummaryForBuilds which

=== modified file 'lib/lp/soyuz/interfaces/binarypackagebuild.py'
--- lib/lp/soyuz/interfaces/binarypackagebuild.py	2010-06-14 09:31:26 +0000
+++ lib/lp/soyuz/interfaces/binarypackagebuild.py	2010-08-10 11:01:15 +0000
@@ -276,7 +276,8 @@
         :return: a `ResultSet` representing the requested builds.
         """
 
-    def getBuildsByArchIds(arch_ids, status=None, name=None, pocket=None):
+    def getBuildsByArchIds(distribution, arch_ids, status=None, name=None,
+                           pocket=None):
         """Retrieve Build Records for a given arch_ids list.
 
         Optionally, for a given status and/or pocket, if ommited return all

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2010-08-02 02:13:52 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2010-08-10 11:01:15 +0000
@@ -878,8 +878,8 @@
             BinaryPackageBuild.select(
                 clause, clauseTables=clauseTables, orderBy=orderBy))
 
-    def getBuildsByArchIds(self, arch_ids, status=None, name=None,
-                           pocket=None):
+    def getBuildsByArchIds(self, distribution, arch_ids, status=None,
+                           name=None, pocket=None):
         """See `IBinaryPackageBuildSet`."""
         # If not distroarchseries was found return empty list
         if not arch_ids:
@@ -938,12 +938,9 @@
 
         # Only pick builds from the distribution's main archive to
         # exclude PPA builds
-        clauseTables.append("Archive")
-        condition_clauses.append("""
-            Archive.purpose IN (%s) AND
-            Archive.id = PackageBuild.archive
-            """ % ','.join(
-                sqlvalues(ArchivePurpose.PRIMARY, ArchivePurpose.PARTNER)))
+        condition_clauses.append(
+            "PackageBuild.archive IN %s" %
+            sqlvalues(list(distribution.all_distro_archive_ids)))
 
         return self._decorate_with_prejoins(
             BinaryPackageBuild.select(' AND '.join(condition_clauses),

=== modified file 'lib/lp/soyuz/model/distroarchseries.py'
--- lib/lp/soyuz/model/distroarchseries.py	2010-06-21 07:26:51 +0000
+++ lib/lp/soyuz/model/distroarchseries.py	2010-08-10 11:01:15 +0000
@@ -245,7 +245,8 @@
         # Use the facility provided by IBinaryPackageBuildSet to
         # retrieve the records.
         return getUtility(IBinaryPackageBuildSet).getBuildsByArchIds(
-            [self.id], build_state, name, pocket)
+            self.distroseries.distribution, [self.id], build_state, name,
+            pocket)
 
     def getReleasedPackages(self, binary_name, pocket=None,
                             include_pending=False, exclude_pocket=None,