launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-803317 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #803317 in Launchpad itself: "DistroSeries:+queue timeout in builds_dict for non-NEW queues"
  https://bugs.launchpad.net/launchpad/+bug/803317

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-803317/+merge/66283

= Summary =

The DistroSeries:+queue page times out on staging and dogfood when I try to look at, for example, the Accepted queue.  This makes Q/A hard.


== Proposed fix ==

Stop making the page slow.  The prejoins were weighing down a query, skewing it towards a sequential scan of LibraryFileAlias (which is rather large).

In this branch I remove the prejoin, and add a couple of separate load_related calls that do the same preloading at the cost of two extra queries.

For a query that previously didn't finish in reasonable time (say 15 minutes), I now get a timing of around 150 milliseconds.


== Pre-implementation notes ==

The interface defined a foreign-key type as Int rather than Reference.  This was wrong, says Julian.


== Implementation details ==

I added the load_related calls into a new utility method.  I didn't want to add them to the method that fetches the original data, since that would require immediate invalidation (which might cause trouble for count queries and such, depending on code structure).  The import fascist didn't want me to put them in the browser code either, and it's probably right.


== Tests ==

{{{
./bin/test -vvc lp.soyuz.tests.test_binarypackagefile
}}}


== Demo and Q/A ==

Go to https://staging.launchpad.net/ubuntu/oneiric/+queue (or equivalent for dogfood) and select other queues besides the default NEW.  They should not time out.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/tests/test_binarypackagefile.py
  lib/lp/soyuz/interfaces/files.py
  lib/lp/soyuz/browser/queue.py
  lib/lp/soyuz/model/files.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-803317/+merge/66283
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-803317 into lp:launchpad.

=== modified file 'lib/lp/soyuz/browser/queue.py'
--- lib/lp/soyuz/browser/queue.py	2011-06-23 13:05:52 +0000
+++ lib/lp/soyuz/browser/queue.py	2011-06-29 11:47:24 +0000
@@ -214,6 +214,7 @@
                     upload.status != PackageUploadStatus.DONE)]
         binary_file_set = getUtility(IBinaryPackageFileSet)
         binary_files = binary_file_set.getByPackageUploadIDs(upload_ids)
+        binary_file_set.loadLibraryFiles(binary_files)
         packageuploadsources = load_referencing(
             PackageUploadSource, uploads, ['packageuploadID'])
         source_file_set = getUtility(ISourcePackageReleaseFileSet)
@@ -226,6 +227,7 @@
         # Get a dictionary of lists of binary files keyed by upload ID.
         package_upload_builds_dict = self.builds_dict(
             upload_ids, binary_files)
+
         build_upload_files, binary_package_names = self.binary_files_dict(
             package_upload_builds_dict, binary_files)
 

=== modified file 'lib/lp/soyuz/interfaces/files.py'
--- lib/lp/soyuz/interfaces/files.py	2011-06-21 11:24:16 +0000
+++ lib/lp/soyuz/interfaces/files.py	2011-06-29 11:47:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213
@@ -22,6 +22,7 @@
     )
 
 from canonical.launchpad import _
+from canonical.launchpad.interfaces.librarian import ILibraryFileAlias
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
 
 
@@ -36,10 +37,13 @@
             required=True, readonly=False,
             )
 
-    libraryfile = Int(
-            title=_('The library file alias for this .deb'), required=True,
-            readonly=False,
-            )
+    libraryfileID = Int(
+            title=_("The LibraryFileAlias id for this .deb"), required=True,
+            readonly=True)
+
+    libraryfile = Reference(
+        ILibraryFileAlias, title=_("The library file alias for this .deb"),
+        required=True, readonly=False)
 
     filetype = Int(
             title=_('The type of this file'), required=True, readonly=False,
@@ -52,6 +56,14 @@
     def getByPackageUploadIDs(package_upload_ids):
         """Return `BinaryPackageFile`s for the `PackageUpload` IDs."""
 
+    def loadLibraryFiles(binary_files):
+        """Bulk-load Librarian files associated with `binary_files`.
+
+        This loads the `LibraryFileAlias` and `LibraryFileContent` for each
+        of `binary_files` into the ORM cache, and returns an iterable of
+        `LibraryFileAlias`.
+        """
+
 
 class ISourcePackageReleaseFile(Interface):
     """A source package release to librarian link record."""

=== modified file 'lib/lp/soyuz/model/files.py'
--- lib/lp/soyuz/model/files.py	2010-08-24 11:31:13 +0000
+++ lib/lp/soyuz/model/files.py	2011-06-29 11:47:24 +0000
@@ -19,7 +19,12 @@
     SQLBase,
     sqlvalues,
     )
+from canonical.launchpad.database.librarian import (
+    LibraryFileAlias,
+    LibraryFileContent,
+    )
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
+from lp.services.database.bulk import load_related
 from lp.soyuz.enums import BinaryPackageFileType
 from lp.soyuz.interfaces.files import (
     IBinaryPackageFile,
@@ -61,9 +66,14 @@
             clauseTables=["PackageUpload", "PackageUploadBuild",
                           "BinaryPackageBuild", "BinaryPackageRelease"],
             prejoins=["binarypackagerelease", "binarypackagerelease.build",
-                      "libraryfile", "libraryfile.content",
                       "binarypackagerelease.binarypackagename"])
 
+    def loadLibraryFiles(self, binary_files):
+        """See `IBinaryPackageFileSet`."""
+        lfas = load_related(LibraryFileAlias, binary_files, ['libraryfileID'])
+        load_related(LibraryFileContent, lfas, ['contentID'])
+        return lfas
+
 
 class SourceFileMixin:
     """Mix-in class for common functionality between source file classes."""

=== added file 'lib/lp/soyuz/tests/test_binarypackagefile.py'
--- lib/lp/soyuz/tests/test_binarypackagefile.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_binarypackagefile.py	2011-06-29 11:47:24 +0000
@@ -0,0 +1,26 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Unit tests for `IBinaryPackageFile` and `IBinaryPackageFileSet`."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.soyuz.interfaces.files import IBinaryPackageFileSet
+from lp.testing import TestCaseWithFactory
+from lp.testing.matchers import Provides
+
+
+class TestBinaryPackageFileSet(TestCaseWithFactory):
+    layer = LaunchpadZopelessLayer
+
+    def test_implements_interface(self):
+        file_set = getUtility(IBinaryPackageFileSet)
+        self.assertThat(file_set, Provides(IBinaryPackageFileSet))
+
+    def test_loadLibraryFiles_returns_associated_lfas(self):
+        bpf = self.factory.makeBinaryPackageFile()
+        lfas = getUtility(IBinaryPackageFileSet).loadLibraryFiles([bpf])
+        self.assertContentEqual([bpf.libraryfile], lfas)