launchpad-reviewers team mailing list archive

[Edwin Grubbs <edwin.grubbs@xxxxxxxxxxxxx>]

Edwin Grubbs has proposed merging lp:~edwin-grubbs/launchpad/bug-663857-product-packages-timeout-part2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #663857 Product:+packages timesout with no packages and one series
  https://bugs.launchpad.net/bugs/663857

For more details, see:
https://code.launchpad.net/~edwin-grubbs/launchpad/bug-663857-product-packages-timeout-part2/+merge/46985

Summary
-------

The $product/+packages still times out because the
ProductPackagesView.distro_packaging property would iterate over all the
product's series in order to find the product's packages.

Implementation details
----------------------

Decorated the series in series_batch so that series/packagings in the
template doesn't execute a query for storm's __nonzero__() method
repeatedly. Switched distro_packaging to use the new Product.packaging
property.
    lib/lp/registry/browser/product.py
    lib/lp/registry/interfaces/product.py
    lib/lp/registry/model/product.py

Cut in half the number of queries for the currentrelease.
    lib/lp/registry/templates/product-packages.pt

Tests
-----

./bin/test -vv -t 'xx-product-package-pages.txt|packaging-views.txt'

Demo and Q/A
------------

* Open https://qastaging.launchpad.net/obsolete-junk/+packages
  * It should not timeout.

Lint
----

Not going to fix this.

./lib/lp/registry/interfaces/product.py
     983: E301 expected 1 blank line, found 2
-- 
https://code.launchpad.net/~edwin-grubbs/launchpad/bug-663857-product-packages-timeout-part2/+merge/46985
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~edwin-grubbs/launchpad/bug-663857-product-packages-timeout-part2 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py	2011-01-13 20:57:45 +0000
+++ lib/lp/registry/browser/product.py	2011-01-20 21:55:28 +0000
@@ -818,6 +818,11 @@
             # This is normal presentation.
             return ''
 
+    @cachedproperty
+    def packagings(self):
+        """Convert packagings to list to prevent multiple evaluations."""
+        return list(self.series.packagings)
+
 
 class SeriesWithReleases(DecoratedSeries):
     """A decorated series that includes releases.
@@ -1119,8 +1124,9 @@
     @cachedproperty
     def series_batch(self):
         """A batch of series that are active or have packages."""
-        return BatchNavigator(
-            self.context.active_or_packaged_series, self.request)
+        decorated_series = DecoratedResultSet(
+            self.context.active_or_packaged_series, DecoratedSeries)
+        return BatchNavigator(decorated_series, self.request)
 
     @property
     def distro_packaging(self):
@@ -1132,18 +1138,8 @@
         title, and an attribute "packagings" which is a list of the relevant
         packagings for this distro and product.
         """
-        # First get a list of all relevant packagings.
-        all_packagings = []
-        for series in self.context.series:
-            for packaging in series.packagings:
-                all_packagings.append(packaging)
-        # We sort it so that the packagings will always be displayed in the
-        # distroseries version, then productseries name order.
-        all_packagings.sort(key=lambda a: (a.distroseries.version,
-            a.productseries.name, a.id))
-
         distros = {}
-        for packaging in all_packagings:
+        for packaging in self.context.packagings:
             distribution = packaging.distroseries.distribution
             if distribution.name in distros:
                 distro = distros[distribution.name]

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2011-01-13 06:07:03 +0000
+++ lib/lp/registry/interfaces/product.py	2011-01-20 21:55:28 +0000
@@ -735,6 +735,8 @@
     active_or_packaged_series = Attribute(
         _("Series that are active and/or have been packaged."))
 
+    packagings = Attribute(_("All the packagings for the project."))
+
     def getVersionSortedSeries(statuses=None, filter_statuses=None):
         """Return all the series sorted by the name field as a version.
 

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2011-01-13 21:03:14 +0000
+++ lib/lp/registry/model/product.py	2011-01-20 21:55:28 +0000
@@ -821,6 +821,22 @@
         return result
 
     @property
+    def packagings(self):
+        store = Store.of(self)
+        result = store.find(
+            (Packaging, DistroSeries),
+            Packaging.distroseries == DistroSeries.id,
+            Packaging.productseries == ProductSeries.id,
+            ProductSeries.product == self)
+        result = result.order_by(
+            DistroSeries.version, ProductSeries.name, Packaging.id)
+
+        def decorate(row):
+            packaging, distroseries = row
+            return packaging
+        return DecoratedResultSet(result, decorate)
+
+    @property
     def name_with_project(self):
         """See lib.canonical.launchpad.interfaces.IProduct"""
         if self.project and self.project.name != self.name:

=== modified file 'lib/lp/registry/templates/product-packages.pt'
--- lib/lp/registry/templates/product-packages.pt	2011-01-14 02:47:41 +0000
+++ lib/lp/registry/templates/product-packages.pt	2011-01-20 21:55:28 +0000
@@ -136,9 +136,11 @@
               >Apache</a>
           </td>
           <td>
-            <span tal:condition="pkging/sourcepackage/currentrelease"
-                    tal:replace="pkging/sourcepackage/currentrelease/version"
-                    >2.3.4-1</span>
+            <span
+                tal:define="currentrelease pkging/sourcepackage/currentrelease"
+                tal:condition="currentrelease"
+                tal:replace="currentrelease/version"
+                >2.3.4-1</span>
           </td>
           <td>
             <a tal:replace="structure pkging/productseries/fmt:link" />