launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16350
[Merge] lp:~wgrant/launchpad/bug-1186349 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-1186349 into lp:launchpad.
Commit message:
Batch DistributionSourcePackage:+changelog.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1186349 in Launchpad itself: "DistributionSourcePackage:+changelog times out on huge histories"
https://bugs.launchpad.net/launchpad/+bug/1186349
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1186349/+merge/201381
DistributionSourcePackage:+changelog currently shows the entire history of the package within the distribution, which nowadays can easily mean several hundred records -- and a timeout. This branch adjusts +changelog to show batches of 50 at a time, mostly rendering in around a second.
Pretty straightforward change, though I had to fix a couple of underlying methods to return ResultSets for efficient slicing. As part of that work, I added support for bulk_decorators to DecoratedResultSet. This new function is a combination of pre_iter_hook and result_decorator, and I intend to abolish pre_iter_hook in favour of bulk_decorator in a later branch.
--
https://code.launchpad.net/~wgrant/launchpad/bug-1186349/+merge/201381
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1186349 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/distributionsourcepackage.py'
--- lib/lp/registry/browser/distributionsourcepackage.py 2013-10-01 05:54:09 +0000
+++ lib/lp/registry/browser/distributionsourcepackage.py 2014-01-13 12:42:33 +0000
@@ -273,6 +273,7 @@
class DistributionSourcePackageBaseView(LaunchpadView):
"""Common features to all `DistributionSourcePackage` views."""
+ @property
def releases(self):
"""All releases for this `IDistributionSourcePackage`."""
@@ -281,40 +282,38 @@
text is not None and isinstance(text, basestring)
and len(text.strip()) > 0)
- dspr_pubs = self.context.getReleasesAndPublishingHistory()
-
- # Return as early as possible to avoid unnecessary processing.
- if len(dspr_pubs) == 0:
- return []
-
- sprs = [dspr.sourcepackagerelease for (dspr, spphs) in dspr_pubs]
- # Preload email/person data only if user is logged on. In the opposite
- # case the emails in the changelog will be obfuscated anyway and thus
- # cause no database lookups.
- the_changelog = '\n'.join(
- [spr.changelog_entry for spr in sprs
- if not_empty(spr.changelog_entry)])
- if self.user:
- self._person_data = dict(
- [(email.email, person) for (email, person) in
- getUtility(IPersonSet).getByEmails(
- extract_email_addresses(the_changelog),
- include_hidden=False)])
- else:
- self._person_data = None
- # Collate diffs for relevant SourcePackageReleases
- pkg_diffs = getUtility(IPackageDiffSet).getDiffsToReleases(
- sprs, preload_for_display=True)
- spr_diffs = {}
- for spr, diffs in itertools.groupby(pkg_diffs,
- operator.attrgetter('to_source')):
- spr_diffs[spr] = list(diffs)
-
- return [
- DecoratedDistributionSourcePackageRelease(
- dspr, spphs, spr_diffs.get(dspr.sourcepackagerelease, []),
- self._person_data, self.user)
- for (dspr, spphs) in dspr_pubs]
+ def decorate(dspr_pubs):
+ sprs = [dspr.sourcepackagerelease for (dspr, spphs) in dspr_pubs]
+ # Preload email/person data only if user is logged on. In
+ # the opposite case the emails in the changelog will be
+ # obfuscated anyway and thus cause no database lookups.
+ the_changelog = '\n'.join(
+ [spr.changelog_entry for spr in sprs
+ if not_empty(spr.changelog_entry)])
+ if self.user:
+ self._person_data = dict(
+ [(email.email, person) for (email, person) in
+ getUtility(IPersonSet).getByEmails(
+ extract_email_addresses(the_changelog),
+ include_hidden=False)])
+ else:
+ self._person_data = None
+ # Collate diffs for relevant SourcePackageReleases
+ pkg_diffs = getUtility(IPackageDiffSet).getDiffsToReleases(
+ sprs, preload_for_display=True)
+ spr_diffs = {}
+ for spr, diffs in itertools.groupby(
+ pkg_diffs, operator.attrgetter('to_source')):
+ spr_diffs[spr] = list(diffs)
+
+ return [
+ DecoratedDistributionSourcePackageRelease(
+ dspr, spphs, spr_diffs.get(dspr.sourcepackagerelease, []),
+ self._person_data, self.user)
+ for (dspr, spphs) in dspr_pubs]
+ return DecoratedResultSet(
+ self.context.getReleasesAndPublishingHistory(),
+ bulk_decorator=decorate)
class DistributionSourcePackageView(DistributionSourcePackageBaseView,
@@ -577,6 +576,10 @@
def label(self):
return 'Change log for %s' % self.context.title
+ @property
+ def batchnav(self):
+ return BatchNavigator(self.releases, self.request)
+
class PublishingHistoryViewMixin:
"""Mixin for presenting batches of `SourcePackagePublishingHistory`s."""
=== modified file 'lib/lp/registry/model/distributionsourcepackage.py'
--- lib/lp/registry/model/distributionsourcepackage.py 2013-10-01 00:57:56 +0000
+++ lib/lp/registry/model/distributionsourcepackage.py 2014-01-13 12:42:33 +0000
@@ -11,7 +11,10 @@
]
import itertools
-import operator
+from operator import (
+ attrgetter,
+ itemgetter,
+ )
from threading import local
from bzrlib.lru_cache import LRUCache
@@ -34,7 +37,6 @@
from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
from lp.bugs.model.bugtarget import BugTargetBase
-from lp.bugs.model.bugtask import BugTask
from lp.bugs.model.structuralsubscription import (
StructuralSubscriptionTargetMixin,
)
@@ -54,6 +56,7 @@
SourcePackage,
SourcePackageQuestionTargetMixin,
)
+from lp.services.database.bulk import load
from lp.services.database.decoratedresultset import DecoratedResultSet
from lp.services.database.interfaces import IStore
from lp.services.database.sqlbase import sqlvalues
@@ -397,35 +400,44 @@
res.order_by(
Desc(SourcePackagePublishingHistory.datecreated),
Desc(SourcePackagePublishingHistory.id))
- return DecoratedResultSet(res, operator.itemgetter(0))
+ return DecoratedResultSet(res, itemgetter(0))
def getReleasesAndPublishingHistory(self):
"""See `IDistributionSourcePackage`."""
- store = Store.of(self.distribution)
- result = store.find(
- (SourcePackageRelease, SourcePackagePublishingHistory),
+ pub_constraints = (
+ DistroSeries.distribution == self.distribution,
SourcePackagePublishingHistory.distroseries == DistroSeries.id,
- DistroSeries.distribution == self.distribution,
SourcePackagePublishingHistory.archiveID.is_in(
self.distribution.all_distro_archive_ids),
SourcePackagePublishingHistory.sourcepackagename ==
self.sourcepackagename,
- SourcePackageRelease.id ==
- SourcePackagePublishingHistory.sourcepackagereleaseID)
- result.order_by(
- Desc(SourcePackageRelease.id),
- Desc(SourcePackagePublishingHistory.datecreated),
- Desc(SourcePackagePublishingHistory.id))
-
- # Collate the publishing history by SourcePackageRelease.
- dspr_pubs = []
- for spr, pubs in itertools.groupby(result, operator.itemgetter(0)):
- dspr_pubs.append(
+ )
+
+ # Find distinct SPRs for our SPN in our archives.
+ spr_ids = Store.of(self.distribution).find(
+ SourcePackagePublishingHistory.sourcepackagereleaseID,
+ *pub_constraints
+ ).order_by(
+ Desc(SourcePackagePublishingHistory.sourcepackagereleaseID)
+ ).config(distinct=True)
+
+ def decorate(spr_ids):
+ # Find the SPPHs for each SPR in our result.
+ load(SourcePackageRelease, spr_ids)
+ sprs = [SourcePackageRelease.get(spr_id) for spr_id in spr_ids]
+ pubs = DistributionSourcePackageRelease.getPublishingHistories(
+ self.distribution, sprs)
+ sprs_by_id = dict(
+ (spr, list(pubs)) for (spr, pubs) in
+ itertools.groupby(pubs, attrgetter('sourcepackagereleaseID')))
+ return [
(DistributionSourcePackageRelease(
- distribution=self.distribution,
- sourcepackagerelease=spr),
- [spph for (spr, spph) in pubs]))
- return dspr_pubs
+ distribution=self.distribution,
+ sourcepackagerelease=spr),
+ sprs_by_id[spr.id])
+ for spr in sprs]
+
+ return DecoratedResultSet(spr_ids, bulk_decorator=decorate)
# XXX kiko 2006-08-16: Bad method name, no need to be a property.
@property
=== modified file 'lib/lp/registry/templates/distributionsourcepackage-changelog.pt'
--- lib/lp/registry/templates/distributionsourcepackage-changelog.pt 2009-09-22 13:11:13 +0000
+++ lib/lp/registry/templates/distributionsourcepackage-changelog.pt 2014-01-13 12:42:33 +0000
@@ -11,10 +11,12 @@
<div metal:fill-slot="main">
<div class="top-portlet">
+ <tal:navigation content="structure view/batchnav/@@+navigation-links-upper" />
<tal:block
- repeat="dspr view/releases"
+ repeat="dspr view/batchnav/currentBatch"
replace="structure dspr/@@+changes">
</tal:block>
+ <tal:navigation content="structure view/batchnav/@@+navigation-links-lower" />
</div>
</div>
=== modified file 'lib/lp/services/database/decoratedresultset.py'
--- lib/lp/services/database/decoratedresultset.py 2013-01-07 02:40:55 +0000
+++ lib/lp/services/database/decoratedresultset.py 2014-01-13 12:42:33 +0000
@@ -37,7 +37,7 @@
delegates(IResultSet, context='result_set')
def __init__(self, result_set, result_decorator=None, pre_iter_hook=None,
- slice_info=False, return_both=False):
+ bulk_decorator=None, slice_info=False, return_both=False):
"""
Wrap `result_set` in a decorator.
@@ -46,19 +46,29 @@
:param result_set: The original result set to be decorated.
:param result_decorator: A transformation function that individual
- results will be passed through.
+ results will be passed through. Mutually exclusive with
+ bulk_decorator.
:param pre_iter_hook: The method to be called (with the 'result_set')
immediately before iteration starts. The return value of the hook
is ignored.
+ :param bulk_decorator: A transformation function that chunks of
+ results will be passed through. Mutually exclusive with
+ result_decorator.
:param slice_info: If True pass information about the slice parameters
to the result_decorator and pre_iter_hook. any() and similar
methods will cause None to be supplied.
:param return_both: If True return both the plain and decorated
values as a tuple.
"""
+ if (bulk_decorator is not None and
+ (result_decorator is not None or pre_iter_hook is not None)):
+ raise TypeError(
+ "bulk_decorator cannot be used with result_decorator or "
+ "pre_iter_hook")
self.result_set = result_set
self.result_decorator = result_decorator
self.pre_iter_hook = pre_iter_hook
+ self.bulk_decorator = bulk_decorator
self.slice_info = slice_info
self.config(return_both=return_both)
@@ -80,7 +90,7 @@
else:
return results, results
- def decorate_or_none(self, result, row_index=None):
+ def decorate_single(self, result, row_index=None):
"""Decorate a result or return None if the result is itself None"""
# If we have a nested DecoratedResultSet we need to propogate
# the plain result.
@@ -91,10 +101,17 @@
else:
if self.result_decorator is None:
decorated = result
- elif self.slice_info:
- decorated = self.result_decorator(result, row_index)
- else:
- decorated = self.result_decorator(result)
+ if self.result_decorator is not None:
+ if self.slice_info:
+ decorated = self.result_decorator(result, row_index)
+ else:
+ decorated = self.result_decorator(result)
+ elif self.bulk_decorator is not None:
+ if self.slice_info:
+ [decorated] = self.bulk_decorator(
+ [result], slice(row_index, row_index + 1))
+ else:
+ [decorated] = self.bulk_decorator([result])
if self.return_both:
return (plain, decorated)
else:
@@ -108,7 +125,7 @@
new_result_set = self.result_set.copy(*args, **kwargs)
return DecoratedResultSet(
new_result_set, self.result_decorator, self.pre_iter_hook,
- self.slice_info, self.return_both)
+ self.bulk_decorator, self.slice_info, self.return_both)
def config(self, *args, **kwargs):
"""See `IResultSet`.
@@ -138,19 +155,28 @@
start = 0
stop = start + len(results)
result_slice = slice(start, stop)
- if self.pre_iter_hook is not None:
+ if self.bulk_decorator is not None:
pre_iter_rows = self._extract_plain_and_result(results)[1]
if self.slice_info:
- self.pre_iter_hook(pre_iter_rows, result_slice)
+ results = self.bulk_decorator(pre_iter_rows, result_slice)
else:
- self.pre_iter_hook(pre_iter_rows)
- if self.slice_info:
- start = result_slice.start
- for offset, value in enumerate(results):
- yield self.decorate_or_none(value, offset + start)
- else:
+ results = self.bulk_decorator(pre_iter_rows)
for value in results:
- yield self.decorate_or_none(value)
+ yield value
+ else:
+ if self.pre_iter_hook is not None:
+ pre_iter_rows = self._extract_plain_and_result(results)[1]
+ if self.slice_info:
+ self.pre_iter_hook(pre_iter_rows, result_slice)
+ else:
+ self.pre_iter_hook(pre_iter_rows)
+ if self.slice_info:
+ start = result_slice.start
+ for offset, value in enumerate(results):
+ yield self.decorate_single(value, offset + start)
+ else:
+ for value in results:
+ yield self.decorate_single(value)
def __getitem__(self, *args, **kwargs):
"""See `IResultSet`.
@@ -163,9 +189,9 @@
if IResultSet.providedBy(naked_value):
return DecoratedResultSet(
value, self.result_decorator, self.pre_iter_hook,
- self.slice_info)
+ self.bulk_decorator, self.slice_info)
else:
- return self.decorate_or_none(value)
+ return self.decorate_single(value)
def iterhook_one_elem(self, value):
if value is not None and self.pre_iter_hook is not None:
@@ -178,7 +204,7 @@
"""
value = self.result_set.any(*args, **kwargs)
self.iterhook_one_elem(value)
- return self.decorate_or_none(value)
+ return self.decorate_single(value)
def first(self, *args, **kwargs):
"""See `IResultSet`.
@@ -187,7 +213,7 @@
"""
value = self.result_set.first(*args, **kwargs)
self.iterhook_one_elem(value)
- return self.decorate_or_none(value)
+ return self.decorate_single(value)
def last(self, *args, **kwargs):
"""See `IResultSet`.
@@ -196,7 +222,7 @@
"""
value = self.result_set.last(*args, **kwargs)
self.iterhook_one_elem(value)
- return self.decorate_or_none(value)
+ return self.decorate_single(value)
def one(self, *args, **kwargs):
"""See `IResultSet`.
@@ -205,7 +231,7 @@
"""
value = self.result_set.one(*args, **kwargs)
self.iterhook_one_elem(value)
- return self.decorate_or_none(value)
+ return self.decorate_single(value)
def order_by(self, *args, **kwargs):
"""See `IResultSet`.
@@ -215,7 +241,7 @@
new_result_set = self.result_set.order_by(*args, **kwargs)
return DecoratedResultSet(
new_result_set, self.result_decorator, self.pre_iter_hook,
- self.slice_info, self.return_both)
+ self.bulk_decorator, self.slice_info, self.return_both)
def get_plain_result_set(self):
"""Return the plain Storm result set."""
@@ -237,4 +263,4 @@
new_result_set = self.result_set.find(*args, **kwargs)
return DecoratedResultSet(
new_result_set, self.result_decorator, self.pre_iter_hook,
- self.slice_info, self.return_both)
+ self.bulk_decorator, self.slice_info, self.return_both)
=== modified file 'lib/lp/services/database/doc/decoratedresultset.txt'
--- lib/lp/services/database/doc/decoratedresultset.txt 2012-03-26 23:48:01 +0000
+++ lib/lp/services/database/doc/decoratedresultset.txt 2014-01-13 12:42:33 +0000
@@ -124,7 +124,27 @@
>>> decorated_results.count()
14
-The pre_iter_hook() method is only called once when iteration begins.
+
+== Bulk operations ==
+
+Views or API calls often need to perform operations that are expensive
+when performed separately on each record. DecoratedResultSet's
+bulk_decorator argument permits operations to be performed over large
+chunks of results at once.
+
+ >>> def all_ones(rows):
+ ... print "that's a chunk of %d" % len(rows)
+ ... return (1 for row in rows)
+ >>> drs = DecoratedResultSet(results, bulk_decorator=all_ones)
+ >>> list(drs)
+ that's a chunk of 14
+ [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ >>> drs.any()
+ that's a chunk of 1
+ 1
+
+pre_iter_hook is like bulk_decorator, except that the return value is
+ignored in favour of the original results.
>>> class FakeResultSet(list):
... def count(self, *args, **kwargs):
=== modified file 'lib/lp/soyuz/model/distributionsourcepackagerelease.py'
--- lib/lp/soyuz/model/distributionsourcepackagerelease.py 2013-02-06 07:25:12 +0000
+++ lib/lp/soyuz/model/distributionsourcepackagerelease.py 2014-01-13 12:42:33 +0000
@@ -52,6 +52,22 @@
self.distribution = distribution
self.sourcepackagerelease = sourcepackagerelease
+ @staticmethod
+ def getPublishingHistories(distribution, sprs):
+ from lp.registry.model.distroseries import DistroSeries
+ res = Store.of(distribution).find(
+ SourcePackagePublishingHistory,
+ SourcePackagePublishingHistory.archiveID.is_in(
+ distribution.all_distro_archive_ids),
+ SourcePackagePublishingHistory.distroseriesID == DistroSeries.id,
+ DistroSeries.distribution == distribution,
+ SourcePackagePublishingHistory.sourcepackagereleaseID.is_in(
+ spr.id for spr in sprs))
+ return res.order_by(
+ Desc(SourcePackagePublishingHistory.sourcepackagereleaseID),
+ Desc(SourcePackagePublishingHistory.datecreated),
+ Desc(SourcePackagePublishingHistory.id))
+
@property
def sourcepackage(self):
"""See IDistributionSourcePackageRelease"""
@@ -72,18 +88,8 @@
@property
def publishing_history(self):
"""See IDistributionSourcePackageRelease."""
- from lp.registry.model.distroseries import DistroSeries
- res = Store.of(self.distribution).find(
- SourcePackagePublishingHistory,
- SourcePackagePublishingHistory.archiveID.is_in(
- self.distribution.all_distro_archive_ids),
- SourcePackagePublishingHistory.distroseriesID == DistroSeries.id,
- DistroSeries.distribution == self.distribution,
- SourcePackagePublishingHistory.sourcepackagerelease ==
- self.sourcepackagerelease)
- return res.order_by(
- Desc(SourcePackagePublishingHistory.datecreated),
- Desc(SourcePackagePublishingHistory.id))
+ return self.getPublishingHistories(
+ self.distribution, [self.sourcepackagerelease])
@property
def builds(self):
=== modified file 'lib/lp/soyuz/stories/distribution/xx-distribution-packages.txt'
--- lib/lp/soyuz/stories/distribution/xx-distribution-packages.txt 2012-08-09 11:23:20 +0000
+++ lib/lp/soyuz/stories/distribution/xx-distribution-packages.txt 2014-01-13 12:42:33 +0000
@@ -552,16 +552,18 @@
>>> flush_database_updates()
>>> logout()
-When the page is reload all versions are presented in descending order.
+After a reload the page lists each version, batched in descending order.
>>> anon_browser.reload()
-
>>> print_displayed_versions(anon_browser.contents)
2.3
2.2
2.1
2.0
1.0.9a-4ubuntu1
+
+ >>> anon_browser.getLink('Next').click()
+ >>> print_displayed_versions(anon_browser.contents)
1.0.9a-4
1.0.8-1ubuntu1
Follow ups
