← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~edwin-grubbs/launchpad/bug-490659-series-timeout into lp:launchpad

 

You have been requested to review the proposed merge of lp:~edwin-grubbs/launchpad/bug-490659-series-timeout into lp:launchpad.

Summary
-------

Bug 490659 reports a timeout caused by a project with over 700 releases.
The ProductSeries' milestones and releases attributes both used a python
function to expand the version numbers in the milestone name so that
milestone version numbers such as 1.1, 1.10, and 1.100 would sort
correctly. To avoid loading all 700 releases from the database just to
sort them in python to find the most recent ones, I added the
milestone_sort_key(timestamp dateexpected, text name) postgresql
function, so that the sorting can take place in the db. I also added an
index to the Milestone table using this function.

I have never submitted a db function for review before, so I'm sure that
I'm doing something wrong. For some reason, the configuration I added to
security.cfg doesn't seem to have any effect, so I have to run the
following sql on launchpad_ftest_template and laucnhpad_dev.

GRANT EXECUTE ON FUNCTION milestone_sort_key(timestamp, text) TO PUBLIC;


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

Added milestone_sort_key(timestamp, text) db function.
    database/schema/patch-2207-99-0.sql
    database/schema/security.cfg

Added __len__ to DecoratedResultSet, so that code expecting the model to
return a list won't blow up on the result set.
    lib/canonical/launchpad/components/decoratedresultset.py
    lib/canonical/launchpad/zcml/decoratedresultset.zcml

Changed the productseries detailed-display macro into its own page since
it makes it cleaner to limit the number of milestones and releases in
the view attributes.
    lib/lp/registry/browser/configure.zcml
    lib/lp/registry/browser/productseries.py
    lib/lp/registry/templates/productseries-macros.pt
    lib/lp/registry/templates/productseries-status.pt
    lib/lp/registry/stories/productseries/xx-productseries-series.txt

Converted HasMilestones' milestones and releases attributes to return a
DecoratedResultSet instead of alist.
    lib/lp/registry/model/milestone.py
    lib/lp/registry/browser/tests/productseries-views.txt

Tests
-----

./bin/test -vv -t 'xx-productseries-series.txt|productseries-views.txt'

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

Create a bunch of sample milestones and releases on launchpad.dev:
    INSERT INTO Milestone (name, product, productseries)
    SELECT 'a' || i, (select id from product where name = 'bzr'),
        (select id from productseries where name='trunk' and product = (
                select id from product where name = 'bzr'))
    FROM generate_series(1, 100) as i;

    INSERT INTO Milestone (name, product, productseries, active)
    SELECT
        'rel-' || i,
        (select id from product where name = 'bzr'),
        (select id from productseries where name='trunk' and product = (
                select id from product where name = 'bzr')),
        FALSE
    FROM generate_series(1, 100) as i;

    INSERT INTO ProductRelease (owner, milestone, datereleased)
    SELECT 1, id, now()
    FROM Milestone
    WHERE name ~ 'rel-.*';

* Open http://launchpad.dev/bzr/+series
  * Only 12 milestones and 12 releases should be listed in the gray box
    for trunk.


-- 
https://code.launchpad.net/~edwin-grubbs/launchpad/bug-490659-series-timeout/+merge/32555
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~edwin-grubbs/launchpad/bug-490659-series-timeout into lp:launchpad.
=== added file 'database/schema/patch-2208-01-0.sql'
--- database/schema/patch-2208-01-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2208-01-0.sql	2010-08-20 04:17:52 +0000
@@ -0,0 +1,9 @@
+-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+-- GNU Affero General Public License version 3 (see the file LICENSE).
+SET client_min_messages=ERROR;
+
+CREATE INDEX milestone_dateexpected_name_sort
+ON Milestone
+USING btree (milestone_sort_key(dateexpected, name));
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 01, 0);

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-08-15 11:29:23 +0000
+++ database/schema/security.cfg	2010-08-20 04:17:52 +0000
@@ -17,6 +17,7 @@
 public.person_sort_key(text, text)         = EXECUTE
 public.calculate_bug_heat(integer)         = EXECUTE
 public.debversion_sort_key(text)           = EXECUTE
+public.milestone_sort_key(timestamp without time zone, text) = EXECUTE
 public.null_count(anyarray)                = EXECUTE
 public.valid_name(text)                    = EXECUTE
 public.valid_bug_name(text)                = EXECUTE

=== modified file 'database/schema/trusted.sql'
--- database/schema/trusted.sql	2010-08-06 09:32:02 +0000
+++ database/schema/trusted.sql	2010-08-20 04:17:52 +0000
@@ -1705,3 +1705,30 @@
 
     return int(total_heat)
 $$;
+
+-- This function is not STRICT, since it needs to handle
+-- dateexpected when it is NULL.
+CREATE OR REPLACE FUNCTION milestone_sort_key(
+    dateexpected timestamp, name text)
+    RETURNS text
+AS $_$
+    # If this method is altered, then any functional indexes using it
+    # need to be rebuilt.
+    import re
+    import datetime
+
+    date_expected, name = args
+
+    def substitude_filled_numbers(match):
+        return match.group(0).zfill(5)
+
+    name = re.sub(u'\d+', substitude_filled_numbers, name)
+    if date_expected is None:
+        # NULL dates are considered to be in the future.
+        date_expected = datetime.datetime(datetime.MAXYEAR, 1, 1)
+    return '%s %s' % (date_expected, name)
+$_$
+LANGUAGE plpythonu IMMUTABLE;
+
+COMMENT ON FUNCTION milestone_sort_key(timestamp, text) IS
+'Sort by the Milestone dateexpected and name. If the dateexpected is NULL, then it is converted to a date far in the future, so it will be sorted as a milestone in the future.';

=== modified file 'lib/lp/registry/browser/__init__.py'
--- lib/lp/registry/browser/__init__.py	2010-08-16 18:39:11 +0000
+++ lib/lp/registry/browser/__init__.py	2010-08-20 04:17:52 +0000
@@ -76,7 +76,7 @@
     @property
     def milestone_table_class(self):
         """The milestone table will be unseen if there are no milestones."""
-        if len(self.context.all_milestones) > 0:
+        if self.context.has_milestones:
             return 'listing'
         else:
             # The page can remove the 'unseen' class to make the table

=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml	2010-08-13 21:30:24 +0000
+++ lib/lp/registry/browser/configure.zcml	2010-08-20 04:17:52 +0000
@@ -1655,6 +1655,13 @@
     </browser:pages>
     <browser:page
         for="lp.registry.interfaces.productseries.IProductSeries"
+        class="lp.registry.browser.productseries.ProductSeriesDetailedDisplayView"
+        template="../templates/productseries-detailed-display.pt"
+        facet="overview"
+        permission="zope.Public"
+        name="+detailed-display"/>
+    <browser:page
+        for="lp.registry.interfaces.productseries.IProductSeries"
         class="lp.registry.browser.productseries.ProductSeriesRdfView"
         facet="overview"
         permission="zope.Public"

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2010-08-11 15:35:29 +0000
+++ lib/lp/registry/browser/productseries.py	2010-08-20 04:17:52 +0000
@@ -10,6 +10,7 @@
     'ProductSeriesBreadcrumb',
     'ProductSeriesBugsMenu',
     'ProductSeriesDeleteView',
+    'ProductSeriesDetailedDisplayView',
     'ProductSeriesEditView',
     'ProductSeriesFacets',
     'ProductSeriesFileBugRedirect',
@@ -436,6 +437,19 @@
         return None
 
 
+class ProductSeriesDetailedDisplayView(ProductSeriesView):
+
+    @cachedproperty
+    def latest_milestones(self):
+        # Convert to list to avoid the query being run multiple times.
+        return list(self.context.milestones[:12])
+
+    @cachedproperty
+    def latest_releases(self):
+        # Convert to list to avoid the query being run multiple times.
+        return list(self.context.releases[:12])
+
+
 class ProductSeriesUbuntuPackagingView(LaunchpadFormView):
 
     schema = IPackaging

=== modified file 'lib/lp/registry/browser/tests/productseries-views.txt'
--- lib/lp/registry/browser/tests/productseries-views.txt	2010-07-30 20:00:59 +0000
+++ lib/lp/registry/browser/tests/productseries-views.txt	2010-08-20 04:17:52 +0000
@@ -307,7 +307,7 @@
 specifications that will be unassigned, and release files that will be
 deleted are available.
 
-    >>> view.milestones
+    >>> list(view.milestones)
     []
     >>> view.bugtasks
     []

=== modified file 'lib/lp/registry/interfaces/milestone.py'
--- lib/lp/registry/interfaces/milestone.py	2010-08-12 18:11:47 +0000
+++ lib/lp/registry/interfaces/milestone.py	2010-08-20 04:17:52 +0000
@@ -226,6 +226,8 @@
     """An interface for classes providing milestones."""
     export_as_webservice_entry()
 
+    has_milestones = Bool(title=_("Whether the object has any milestones."))
+
     milestones = exported(
         CollectionField(
             title=_("The visible and active milestones associated with this "

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2010-08-02 02:13:52 +0000
+++ lib/lp/registry/model/milestone.py	2010-08-20 04:17:52 +0000
@@ -21,8 +21,13 @@
 from sqlobject import (
     AND, BoolCol, DateCol, ForeignKey, SQLMultipleJoin, SQLObjectNotFound,
     StringCol)
-from storm.locals import And, Store
-
+
+
+from storm.expr import And
+from storm.store import Store
+
+
+from canonical.cachedproperty import cachedproperty
 from canonical.database.sqlbase import SQLBase, sqlvalues
 from canonical.launchpad.webapp.sorting import expand_numbers
 from lp.app.errors import NotFoundError
@@ -63,6 +68,9 @@
 class HasMilestonesMixin:
     implements(IHasMilestones)
 
+    _milestone_order = (
+        'milestone_sort_key(Milestone.dateexpected, Milestone.name) DESC')
+
     def _getMilestoneCondition(self):
         """Provides condition for milestones and all_milestones properties.
 
@@ -74,11 +82,15 @@
             "Unexpected class for mixin: %r" % self)
 
     @property
+    def has_milestones(self):
+        return self.all_milestones.any() is not None
+
+    @property
     def all_milestones(self):
         """See `IHasMilestones`."""
         store = Store.of(self)
         result = store.find(Milestone, self._getMilestoneCondition())
-        return sorted(result, key=milestone_sort_key, reverse=True)
+        return result.order_by(self._milestone_order)
 
     @property
     def milestones(self):
@@ -87,7 +99,7 @@
         result = store.find(Milestone,
                             And(self._getMilestoneCondition(),
                                 Milestone.active == True))
-        return sorted(result, key=milestone_sort_key, reverse=True)
+        return result.order_by(self._milestone_order)
 
 
 class Milestone(SQLBase, StructuralSubscriptionTargetMixin, HasBugsBase):

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2010-08-02 02:13:52 +0000
+++ lib/lp/registry/model/projectgroup.py	2010-08-20 04:17:52 +0000
@@ -54,7 +54,7 @@
 from lp.services.worlddata.model.language import Language
 from lp.registry.model.mentoringoffer import MentoringOffer
 from lp.registry.model.milestone import (
-    Milestone, ProjectMilestone, milestone_sort_key)
+    Milestone, ProjectMilestone)
 from lp.registry.model.announcement import MakesAnnouncements
 from lp.registry.model.pillar import HasAliasMixin
 from lp.registry.model.product import Product
@@ -381,10 +381,25 @@
         result.group_by(Milestone.name)
         if only_active:
             result.having('BOOL_OR(Milestone.active) = TRUE')
-        milestones = shortlist(
+        # MIN(Milestone.dateexpected) has to be used to match the
+        # aggregate function in the `columns` variable.
+        result.order_by(
+            'milestone_sort_key(MIN(Milestone.dateexpected), Milestone.name) '
+            'DESC')
+        return shortlist(
             [ProjectMilestone(self, name, dateexpected, active)
              for name, dateexpected, active in result])
-        return sorted(milestones, key=milestone_sort_key, reverse=True)
+
+    @property
+    def has_milestones(self):
+        """See `IHasMilestones`."""
+        store = Store.of(self)
+        result = store.find(
+            Milestone.id,
+            And(Milestone.product == Product.id,
+                Product.project == self,
+                Product.active == True))
+        return result.any() is not None
 
     @property
     def milestones(self):

=== modified file 'lib/lp/registry/stories/productseries/xx-productseries-series.txt'
--- lib/lp/registry/stories/productseries/xx-productseries-series.txt	2010-05-24 20:23:19 +0000
+++ lib/lp/registry/stories/productseries/xx-productseries-series.txt	2010-08-20 04:17:52 +0000
@@ -29,7 +29,7 @@
     >>> series_trunk = find_tag_by_id(content, 'series-trunk')
     >>> print extract_text(series_trunk)
     trunk series Focus of Development
-    Milestones: 1.0    Releases: 0.9.2, 0.9.1, 0.9
+    Latest milestones: 1.0    Latest releases: 0.9.2, 0.9.1, 0.9
     Bugs targeted: None
     Blueprints targeted: 1 Unknown
     The "trunk" series represents the primary line of development rather ...
@@ -46,7 +46,7 @@
     >>> series_1_0 = find_tag_by_id(content, 'series-1-0')
     >>> print extract_text(series_1_0)
     1.0 series Active Development
-    Releases: 1.0.0
+    Latest releases: 1.0.0
     Bugs targeted: 1 New
     Blueprints targeted: None
     The 1.0 branch of the Mozilla web browser. Currently, this is the ...

=== modified file 'lib/lp/registry/templates/object-milestones.pt'
--- lib/lp/registry/templates/object-milestones.pt	2009-11-07 00:49:26 +0000
+++ lib/lp/registry/templates/object-milestones.pt	2010-08-20 04:17:52 +0000
@@ -28,7 +28,7 @@
   <tal:milestones define="milestones context/all_milestones">
     <table id="milestones" class="listing"
       tal:define="has_series context/series|nothing"
-      tal:condition="milestones">
+      tal:condition="context/has_milestones">
       <thead>
         <tr>
           <th>Version</th>
@@ -45,7 +45,7 @@
       </tbody>
     </table>
 
-    <tal:no-milestones condition="not: milestones">
+    <tal:no-milestones condition="not: context/has_milestones">
       <p>There are no milestones associated with
         <span tal:replace="context/title" />
       </p>

=== modified file 'lib/lp/registry/templates/productseries-macros.pt'
--- lib/lp/registry/templates/productseries-macros.pt	2010-05-24 20:23:19 +0000
+++ lib/lp/registry/templates/productseries-macros.pt	2010-08-20 04:17:52 +0000
@@ -4,48 +4,6 @@
   xmlns:i18n="http://xml.zope.org/namespaces/i18n";
   omit-tag="">
 
-<metal:detailed_display define-macro="detailed_display">
-  <tal:comment replace="nothing">
-    This macro expects two variables to be defined:
-      - 'series': The ProductSeries
-      - 'is_focus': A boolean saying whether this is the series in which
-                    development is focused.
-  </tal:comment>
-
-  <strong><a tal:attributes="href series/fmt:url"
-             tal:content="series/name" >
-      1.0
-    </a> series
-  </strong>
-  <em tal:condition="is_focus">Focus of Development</em>
-  <em tal:condition="not: is_focus" tal:content="series/status/title">
-    This series' status
-  </em>
-  <div>
-    <tal:milestones condition="series/milestones">
-      Milestones:
-      <tal:milestone repeat="milestone series/milestones">
-        <a tal:attributes="href milestone/fmt:url" tal:content="milestone/name"
-          >name</a><tal:comma condition="not:repeat/milestone/end">,</tal:comma>
-      </tal:milestone>
-    </tal:milestones>
-    <tal:release repeat="release series/releases">
-      <tal:release-start condition="repeat/release/start">
-        <tal:comment condition="nothing">
-          Releases may be a list or an resultset. We cannot easily know if there
-          releases until the first one is returned.
-        </tal:comment>
-        <tal:space condition="series/milestones"> &nbsp &nbsp </tal:space>
-        Releases:
-      </tal:release-start>
-      <a tal:attributes="href release/fmt:url" tal:content="release/version"
-        >version</a><tal:comma condition="not:repeat/release/end">,</tal:comma>
-    </tal:release>
-  </div>
-  <metal:extra define-slot="extra" />
-</metal:detailed_display>
-
-
 <metal:branch_display define-macro="branch_display">
   <tal:comment condition="nothing">
     This macro expects two variables to be defined:

=== modified file 'lib/lp/registry/templates/productseries-status.pt'
--- lib/lp/registry/templates/productseries-status.pt	2009-07-21 18:17:49 +0000
+++ lib/lp/registry/templates/productseries-status.pt	2010-08-20 04:17:52 +0000
@@ -8,8 +8,7 @@
     is_focus context/is_development_focus;
     spec_count_status view/specification_status_counts;"
   >
-  <metal:series use-macro="series/@@+macros/detailed_display">
-    <div metal:fill-slot="extra">
+  <div tal:replace="structure context/@@+detailed-display"/>
       <div>
         <tal:not-obsolete
           condition="not: view/is_obsolete"
@@ -42,6 +41,4 @@
       <tal:summary
         condition="series/summary"
         content="structure context/summary/fmt:text-to-html" />
-    </div>
-  </metal:series>
 </div>