launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00685
[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">     </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>
