Launchpad itself

Merge lp:~edwin-grubbs/launchpad/bug-490659-series-timeout-part2 into lp:launchpad/db-devel

  1. bug-490659-series-timeout-part2
  2. Merge into db-devel
Proposed by Edwin Grubbs
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: 9734
Proposed branch: lp:~edwin-grubbs/launchpad/bug-490659-series-timeout-part2
Merge into: lp:launchpad/db-devel
Diff against target: 288 lines (+120/-13)
10 files modified
lib/lp/registry/browser/distroseries.py (+4/-0)
lib/lp/registry/browser/productseries.py (+6/-1)
lib/lp/registry/browser/tests/test_series_views.py (+42/-4)
lib/lp/registry/interfaces/product.py (+4/-1)
lib/lp/registry/interfaces/productseries.py (+4/-1)
lib/lp/registry/model/milestone.py (+5/-2)
lib/lp/registry/model/productseries.py (+4/-1)
lib/lp/registry/templates/productseries-milestone-table-row.pt (+1/-1)
lib/lp/registry/templates/productseries-table-releases.pt (+6/-2)
lib/lp/registry/tests/test_product.py (+44/-0)
To merge this branch: bzr merge lp:~edwin-grubbs/launchpad/bug-490659-series-timeout-part2
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+34159@code.launchpad.net

Description of the change

Summary
-------

This branch is dependent on the milestone_sort_key() db function that
was recently landed on db-devel. The milestones/releases for a series
are now batched. I also limited the number of milestones/releases
displayed in the timeline graph.

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

Added batching:
    lib/lp/registry/browser/distroseries.py
    lib/lp/registry/browser/productseries.py
    lib/lp/registry/browser/tests/test_distroseries_views.py
    lib/lp/registry/browser/tests/test_productseries_views.py
    lib/lp/registry/model/milestone.py
    lib/lp/registry/templates/productseries-table-releases.pt

Limited the number of milestones/releases displayed in the timeline
graph.
    lib/lp/registry/interfaces/product.py
    lib/lp/registry/interfaces/productseries.py
    lib/lp/registry/model/productseries.py
    lib/lp/registry/tests/test_product.py

The cache TALes needs the third argument to ensure that it retrieves the
right cached value for a block in a loop.
    lib/lp/registry/templates/productseries-milestone-table-row.pt

Tests
-----

$ ./bin/test --list-tests -vv -t 'test_(distro|product)series_views|registry\.tests\.test_product\.'

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

* Open http://launchpad.dev/firefox/trunk

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

A meta-question; is batching here needed? I'm surprised we have enough
data that its a problem (or put another way, whats the scaling factors
at play here; how can we be sure batching will fix the issue).

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

> A meta-question; is batching here needed? I'm surprised we have enough
> data that its a problem (or put another way, whats the scaling factors
> at play here; how can we be sure batching will fix the issue).

The libpng project has 777 releases on a single series. This is caused by the Release URL Pattern on the "main" series which automatically downloads new versions of libpng tarballs and creates a revision to hold the file.

Even projects that don't have that many milestones or releases may have problems displaying all of them since each milestone will display stats on how many bugs and blueprints are assigned to it.

Revision history for this message
Graham Binns (gmb) wrote :

<gmb> EdwinGrubbs, Those two tests are *awfully* similar. Any chance they could be unified in their own TestCase (e.g. TestFooBatchNavigators)
<EdwinGrubbs> gmb: sure
<gmb> EdwinGrubbs, Cool.
 EdwinGrubbs, Other than that, r=me.

review: Approve (code)
Revision history for this message
Robert Collins (lifeless) wrote :

On Wed, Sep 1, 2010 at 2:41 AM, Edwin Grubbs <email address hidden> wrote:

> The libpng project has 777 releases on a single series. This is caused by the Release URL Pattern on the "main" series which automatically downloads new versions of libpng tarballs and creates a revision to hold the file.
>
> Even projects that don't have that many milestones or releases may have problems displaying all of them since each milestone will display stats on how many bugs and blueprints are assigned to it.

Wow. We should be able to make the stats display reasonably fast;
might want to consider tuning the batch size up a bit if we can, e.g.
to 24 (2 years of monthly releases).

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

On Tue, Aug 31, 2010 at 4:28 PM, Robert Collins
<email address hidden> wrote:
> On Wed, Sep 1, 2010 at 2:41 AM, Edwin Grubbs <email address hidden> wrote:
>
>> The libpng project has 777 releases on a single series. This is caused by the Release URL Pattern on the "main" series which automatically downloads new versions of libpng tarballs and creates a revision to hold the file.
>>
>> Even projects that don't have that many milestones or releases may have problems displaying all of them since each milestone will display stats on how many bugs and blueprints are assigned to it.
>
> Wow. We should be able to make the stats display reasonably fast;
> might want to consider tuning the batch size up a bit if we can, e.g.
> to 24 (2 years of monthly releases).

The default batch size on launchpad.dev is 5, but the default batch
size on production is 50. This default is part of the BatchNavigator,
so it doesn't just apply to this page.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/distroseries.py'
2--- lib/lp/registry/browser/distroseries.py 2010-08-20 20:31:18 +0000
3+++ lib/lp/registry/browser/distroseries.py 2010-09-01 00:31:00 +0000
4@@ -360,6 +360,10 @@
5
6 milestone_can_release = False
7
8+ @cachedproperty
9+ def milestone_batch_navigator(self):
10+ return BatchNavigator(self.context.all_milestones, self.request)
11+
12
13 class DistroSeriesEditView(LaunchpadEditFormView, SeriesStatusMixin):
14 """View class that lets you edit a DistroSeries object.
15
16=== modified file 'lib/lp/registry/browser/productseries.py'
17--- lib/lp/registry/browser/productseries.py 2010-08-24 19:06:33 +0000
18+++ lib/lp/registry/browser/productseries.py 2010-09-01 00:31:00 +0000
19@@ -59,6 +59,7 @@
20 from canonical.launchpad import _
21 from canonical.launchpad.helpers import browserLanguages
22 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
23+from canonical.launchpad.webapp.batching import BatchNavigator
24 from canonical.launchpad.webapp import (
25 ApplicationMenu,
26 canonical_url,
27@@ -475,13 +476,17 @@
28 for status in sorted(status_counts,
29 key=attrgetter('sortkey'))]
30
31- @property
32+ @cachedproperty
33 def latest_release_with_download_files(self):
34 for release in self.context.releases:
35 if len(list(release.files)) > 0:
36 return release
37 return None
38
39+ @cachedproperty
40+ def milestone_batch_navigator(self):
41+ return BatchNavigator(self.context.all_milestones, self.request)
42+
43
44 class ProductSeriesDetailedDisplayView(ProductSeriesView):
45
46
47=== renamed file 'lib/lp/registry/browser/tests/test_distroseries_views.py' => 'lib/lp/registry/browser/tests/test_series_views.py'
48--- lib/lp/registry/browser/tests/test_distroseries_views.py 2010-08-20 20:31:18 +0000
49+++ lib/lp/registry/browser/tests/test_series_views.py 2010-09-01 00:31:00 +0000
50@@ -3,13 +3,13 @@
51
52 __metaclass__ = type
53
54-import unittest
55-
56 from storm.zope.interfaces import IResultSet
57 from zope.component import getUtility
58 from zope.security.proxy import removeSecurityProxy
59
60+from canonical.config import config
61 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
62+from canonical.launchpad.webapp.batching import BatchNavigator
63 from canonical.testing import LaunchpadZopelessLayer
64 from lp.testing import TestCaseWithFactory
65 from lp.testing.views import create_initialized_view
66@@ -44,5 +44,43 @@
67 self.assertEqual(view.needs_linking, None)
68
69
70-def test_suite():
71- return unittest.TestLoader().loadTestsFromName(__name__)
72+class TestMilestoneBatchNavigatorAttribute(TestCaseWithFactory):
73+ """Test the series.milestone_batch_navigator attribute."""
74+
75+ layer = LaunchpadZopelessLayer
76+
77+ def test_distroseries_milestone_batch_navigator(self):
78+ ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
79+ distroseries = self.factory.makeDistroSeries(distribution=ubuntu)
80+ for name in ('a', 'b', 'c', 'd'):
81+ distroseries.newMilestone(name)
82+ view = create_initialized_view(distroseries, name='+index')
83+ self._check_milestone_batch_navigator(view)
84+
85+ def test_productseries_milestone_batch_navigator(self):
86+ product = self.factory.makeProduct()
87+ for name in ('a', 'b', 'c', 'd'):
88+ product.development_focus.newMilestone(name)
89+ view = create_initialized_view(
90+ product.development_focus, name='+index')
91+ self._check_milestone_batch_navigator(view)
92+
93+ def _check_milestone_batch_navigator(self, view):
94+ config.push('default-batch-size', """
95+ [launchpad]
96+ default_batch_size: 2
97+ """)
98+ self.assert_(
99+ isinstance(view.milestone_batch_navigator, BatchNavigator),
100+ 'milestone_batch_navigator is not a BatchNavigator object: %r'
101+ % view.milestone_batch_navigator)
102+ self.assertEqual(4, view.milestone_batch_navigator.batch.total())
103+ expected = [
104+ 'd',
105+ 'c',
106+ ]
107+ milestone_names = [
108+ item.name
109+ for item in view.milestone_batch_navigator.currentBatch()]
110+ self.assertEqual(expected, milestone_names)
111+ config.pop('default-batch-size')
112
113=== modified file 'lib/lp/registry/interfaces/product.py'
114--- lib/lp/registry/interfaces/product.py 2010-08-26 23:54:09 +0000
115+++ lib/lp/registry/interfaces/product.py 2010-09-01 00:31:00 +0000
116@@ -786,7 +786,10 @@
117 @export_read_operation()
118 @export_operation_as('get_timeline')
119 def getTimeline(include_inactive):
120- """Return basic timeline data useful for creating a diagram."""
121+ """Return basic timeline data useful for creating a diagram.
122+
123+ The number of milestones returned per series is limited.
124+ """
125
126
127 class IProduct(
128
129=== modified file 'lib/lp/registry/interfaces/productseries.py'
130--- lib/lp/registry/interfaces/productseries.py 2010-08-20 20:31:18 +0000
131+++ lib/lp/registry/interfaces/productseries.py 2010-09-01 00:31:00 +0000
132@@ -308,7 +308,10 @@
133 @export_read_operation()
134 @export_operation_as('get_timeline')
135 def getTimeline(include_inactive):
136- """Return basic timeline data useful for creating a diagram."""
137+ """Return basic timeline data useful for creating a diagram.
138+
139+ The number of milestones returned is limited.
140+ """
141
142
143 class IProductSeries(IProductSeriesEditRestricted, IProductSeriesPublic,
144
145=== modified file 'lib/lp/registry/model/milestone.py'
146--- lib/lp/registry/model/milestone.py 2010-08-24 19:06:33 +0000
147+++ lib/lp/registry/model/milestone.py 2010-09-01 00:31:00 +0000
148@@ -97,7 +97,7 @@
149
150 @property
151 def has_milestones(self):
152- return self.all_milestones.any() is not None
153+ return not self.all_milestones.is_empty()
154
155 @property
156 def all_milestones(self):
157@@ -293,7 +293,10 @@
158 def __init__(self, target, name, dateexpected, active):
159 self.name = name
160 self.code_name = None
161- self.id = None
162+ # The id is necessary for generating a unique memcache key
163+ # in a page template loop. The ProjectMilestone.id is passed
164+ # in as the third argument to the "cache" TALes.
165+ self.id = 'ProjectGroup:%s/Milestone:%s' % (target.name, name)
166 self.code_name = None
167 self.product = None
168 self.distribution = None
169
170=== modified file 'lib/lp/registry/model/productseries.py'
171--- lib/lp/registry/model/productseries.py 2010-08-20 20:31:18 +0000
172+++ lib/lp/registry/model/productseries.py 2010-09-01 00:31:00 +0000
173@@ -97,6 +97,9 @@
174 )
175
176
177+MAX_TIMELINE_MILESTONES = 20
178+
179+
180 def landmark_key(landmark):
181 """Sorts landmarks by date and name."""
182 if landmark['date'] is None:
183@@ -543,7 +546,7 @@
184
185 def getTimeline(self, include_inactive=False):
186 landmarks = []
187- for milestone in self.all_milestones:
188+ for milestone in self.all_milestones[:MAX_TIMELINE_MILESTONES]:
189 if milestone.product_release is None:
190 # Skip inactive milestones, but include releases,
191 # even if include_inactive is False.
192
193=== modified file 'lib/lp/registry/templates/productseries-milestone-table-row.pt'
194--- lib/lp/registry/templates/productseries-milestone-table-row.pt 2010-05-27 04:10:17 +0000
195+++ lib/lp/registry/templates/productseries-milestone-table-row.pt 2010-09-01 00:31:00 +0000
196@@ -5,7 +5,7 @@
197 define="milestone_menu view/milestone/menu:overview;
198 milestone view/milestone;
199 release view/release"
200- tal:content="cache:private, 1 hour">
201+ tal:content="cache:private, 1 hour, milestone/id">
202 <tr>
203 <td>
204 <img src="/@@/milestone" alt="" />
205
206=== modified file 'lib/lp/registry/templates/productseries-table-releases.pt'
207--- lib/lp/registry/templates/productseries-table-releases.pt 2010-05-17 20:09:03 +0000
208+++ lib/lp/registry/templates/productseries-table-releases.pt 2010-09-01 00:31:00 +0000
209@@ -2,8 +2,10 @@
210 xmlns:tal="http://xml.zope.org/namespaces/tal"
211 xmlns:metal="http://xml.zope.org/namespaces/metal"
212 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
213- omit-tag="">
214+ tal:content="cache:anonymous, 1 hour">
215
216+<tal:navigation_top
217+ replace="structure view/milestone_batch_navigator/@@+navigation-links-upper" />
218 <table class="listing"
219 tal:attributes="id context/name/fmt:css-id/series-;
220 class view/milestone_table_class">
221@@ -16,9 +18,11 @@
222 </tr>
223 </thead>
224 <tbody id="milestone-rows">
225- <tal:row repeat="milestone context/all_milestones">
226+ <tal:row repeat="milestone view/milestone_batch_navigator/currentBatch">
227 <tal:milestone replace="structure milestone/@@+productseries-table-row" />
228 </tal:row>
229 </tbody>
230 </table>
231+<tal:navigation_bottom
232+ replace="structure view/milestone_batch_navigator/@@+navigation-links-lower" />
233 </tal:root>
234
235=== modified file 'lib/lp/registry/tests/test_product.py'
236--- lib/lp/registry/tests/test_product.py 2010-08-23 16:05:36 +0000
237+++ lib/lp/registry/tests/test_product.py 2010-09-01 00:31:00 +0000
238@@ -92,6 +92,50 @@
239 expected,
240 list(product.getMilestonesAndReleases()))
241
242+ def test_getTimeline_limit(self):
243+ # Only 20 milestones/releases per series should be included in the
244+ # getTimeline() results. The results are sorted by
245+ # descending dateexpected and name, so the presumed latest
246+ # milestones should be included.
247+ product = self.factory.makeProduct(name='foo')
248+ for i in range(25):
249+ milestone_list = self.factory.makeMilestone(
250+ product=product,
251+ productseries=product.development_focus,
252+ name=str(i))
253+
254+ # 0 through 4 should not be in the list.
255+ expected_milestones = [
256+ '/foo/+milestone/24',
257+ '/foo/+milestone/23',
258+ '/foo/+milestone/22',
259+ '/foo/+milestone/21',
260+ '/foo/+milestone/20',
261+ '/foo/+milestone/19',
262+ '/foo/+milestone/18',
263+ '/foo/+milestone/17',
264+ '/foo/+milestone/16',
265+ '/foo/+milestone/15',
266+ '/foo/+milestone/14',
267+ '/foo/+milestone/13',
268+ '/foo/+milestone/12',
269+ '/foo/+milestone/11',
270+ '/foo/+milestone/10',
271+ '/foo/+milestone/9',
272+ '/foo/+milestone/8',
273+ '/foo/+milestone/7',
274+ '/foo/+milestone/6',
275+ '/foo/+milestone/5',
276+ ]
277+
278+ [series] = product.getTimeline()
279+ timeline_milestones = [
280+ landmark['uri']
281+ for landmark in series['landmarks']]
282+ self.assertEqual(
283+ expected_milestones,
284+ timeline_milestones)
285+
286
287 class TestProductFiles(unittest.TestCase):
288 """Tests for downloadable product files."""

Subscribers

People subscribed via source and target branches

to status/vote changes: