launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01892
[Merge] lp:~lifeless/launchpad/soyuz into lp:launchpad/devel
Robert Collins has proposed merging lp:~lifeless/launchpad/soyuz into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Fix scaling of ppa/+packages to be flat with source packages, distro series and binary packages. I'm not completely confident in the binary package angle as the page is driven by source packages - but its better than nothing.
The key change here is to move an adapter lookup into an eager loaded dataset - the older code which was a problem previously was exacerbated by the underlying changes made to optimise getBuildStatusSummariesForSourceIdsAndArchive. As a result of which I could strip out some code made just for that code path.
--
https://code.launchpad.net/~lifeless/launchpad/soyuz/+merge/40499
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/soyuz into lp:launchpad/devel.
=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py 2010-11-08 12:12:29 +0000
+++ lib/lp/registry/model/distroseries.py 2010-11-10 04:28:49 +0000
@@ -984,16 +984,15 @@
source_package_ids = [
package_name.id for package_name in source_package_names]
releases = SourcePackageRelease.select("""
- SourcePackageName.id IN %s AND
+ SourcePackageRelease.sourcepackagename IN %s AND
SourcePackageRelease.id =
SourcePackagePublishingHistory.sourcepackagerelease AND
SourcePackagePublishingHistory.id = (
SELECT max(spph.id)
FROM SourcePackagePublishingHistory spph,
- SourcePackageRelease spr, SourcePackageName spn
+ SourcePackageRelease spr
WHERE
- spn.id = SourcePackageName.id AND
- spr.sourcepackagename = spn.id AND
+ spr.sourcepackagename = SourcePackageRelease.sourcepackagename AND
spph.sourcepackagerelease = spr.id AND
spph.archive IN %s AND
spph.status IN %s AND
=== modified file 'lib/lp/soyuz/adapters/archivesourcepublication.py'
--- lib/lp/soyuz/adapters/archivesourcepublication.py 2010-11-07 21:54:38 +0000
+++ lib/lp/soyuz/adapters/archivesourcepublication.py 2010-11-10 04:28:49 +0000
@@ -14,11 +14,14 @@
'ArchiveSourcePublications',
]
+from collections import defaultdict
from lazr.delegates import delegates
from zope.component import getUtility
from canonical.launchpad.browser.librarian import ProxiedLibraryFileAlias
+from canonical.launchpad.interfaces.lpstorm import IStore
+from lp.registry.model.sourcepackagename import SourcePackageName
from lp.registry.model.distroseries import DistroSeries
from lp.soyuz.interfaces.publishing import (
IPublishingSet,
@@ -53,11 +56,12 @@
"""
delegates(ISourcePackagePublishingHistory)
- def __init__(self, context, unpublished_builds, builds, changesfile):
+ def __init__(self, context, unpublished_builds, builds, changesfile, status_summary):
self.context = context
self._unpublished_builds = unpublished_builds
self._builds = builds
self._changesfile = changesfile
+ self._status_summary = status_summary
@property
def sourcepackagerelease(self):
@@ -84,16 +88,8 @@
def getStatusSummaryForBuilds(self):
"""See `ISourcePackagePublishingHistory`."""
- # XXX Michael Nelson 2009-05-08 bug=373715. It would be nice if
- # lazr.delegates passed the delegates 'self' for pass-through
- # methods, then we wouldn't need to proxy this method call via the
- # IPublishingSet - instead the delegate would call
- # ISourcePackagePublishingHistory.getStatusSummaryForBuilds() but
- # using the delegate as self - might not be possible without
- # duck-typing.
- return getUtility(
- IPublishingSet).getBuildStatusSummaryForSourcePublication(self,
- self.getBuilds)
+ return self._status_summary
+
class ArchiveSourcePublications:
"""`ArchiveSourcePublication` iterator."""
@@ -162,19 +158,39 @@
return iter(results)
# Load the extra-information for all source publications.
+ # All of this code would be better on an object representing a set of
+ # publications.
builds_by_source = self.getBuildsBySource()
unpublished_builds_by_source = self.getUnpublishedBuildsBySource()
changesfiles_by_source = self.getChangesFileBySource()
+ # Source package names are used by setNewerDistroSeriesVersions:
+ # batch load the used source package names.
+ spn_ids = set()
+ for spph in self._source_publications:
+ spn_ids.add(spph.sourcepackagerelease.sourcepackagenameID)
+ list(IStore(SourcePackageName).find(SourcePackageName,
+ SourcePackageName.id.is_in(spn_ids)))
DistroSeries.setNewerDistroSeriesVersions(self._source_publications)
+ # Load all the build status summaries at once.
+ publishing_set = getUtility(IPublishingSet)
+ archive_pub_ids = defaultdict(list)
+ for pub in self._source_publications:
+ archive_pub_ids[pub.archive].append(pub.id)
+ status_summaries = {}
+ for archive, pub_ids in archive_pub_ids.items():
+ status_summaries.update(
+ publishing_set.getBuildStatusSummariesForSourceIdsAndArchive(
+ pub_ids, archive))
# Build the decorated object with the information we have.
for pub in self._source_publications:
builds = builds_by_source.get(pub, [])
unpublished_builds = unpublished_builds_by_source.get(pub, [])
changesfile = changesfiles_by_source.get(pub, None)
+ status_summary = status_summaries[pub.id]
complete_pub = ArchiveSourcePublication(
pub, unpublished_builds=unpublished_builds, builds=builds,
- changesfile=changesfile)
+ changesfile=changesfile, status_summary=status_summary)
results.append(complete_pub)
return iter(results)
=== modified file 'lib/lp/soyuz/browser/tests/test_archive_packages.py'
--- lib/lp/soyuz/browser/tests/test_archive_packages.py 2010-10-16 15:09:47 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_packages.py 2010-11-10 04:28:49 +0000
@@ -9,19 +9,28 @@
__all__ = [
'TestP3APackages',
'TestPPAPackages',
- 'test_suite',
]
+from testtools.matchers import (
+ Equals,
+ LessThan,
+ MatchesAny,
+ )
from zope.security.interfaces import Unauthorized
+from canonical.launchpad.webapp import canonical_url
from canonical.testing.layers import LaunchpadFunctionalLayer
from lp.soyuz.browser.archive import ArchiveNavigationMenu
from lp.testing import (
login,
login_person,
+ person_logged_in,
TestCaseWithFactory,
)
+from lp.testing.matchers import HasQueryCount
+from lp.testing.sampledata import ADMIN_EMAIL
from lp.testing.views import create_initialized_view
+from lp.testing._webservice import QueryCollector
class TestP3APackages(TestCaseWithFactory):
@@ -113,3 +122,81 @@
def test_specified_name_filter_returns_none_on_empty_filter(self):
view = self.getPackagesView('field.name_filter=')
self.assertIs(None, view.specified_name_filter)
+
+ def test_source_query_counts(self):
+ query_baseline = 42
+ # Get the baseline.
+ ppa = self.factory.makeArchive()
+ collector = QueryCollector()
+ collector.register()
+ self.addCleanup(collector.unregister)
+ ppa = self.factory.makeArchive()
+ viewer = self.factory.makePerson(password="test")
+ browser = self.getUserBrowser(user=viewer)
+ with person_logged_in(viewer):
+ # The baseline has one package, because otherwise the short-circuit
+ # prevents the packages iteration happening at all and we're not
+ # actually measuring scaling appropriately.
+ self.factory.makeSourcePackagePublishingHistory(archive=ppa)
+ url = canonical_url(ppa) + "/+packages"
+ browser.open(url)
+ self.assertThat(collector, HasQueryCount(LessThan(query_baseline)))
+ expected_count = collector.count
+ # We scale with 1 query per distro series because of
+ # getCurrentSourceReleases.
+ expected_count += 1
+ # We need a fuzz of one because if the test is the first to run a
+ # credentials lookup is done as well (and accrued to the collector).
+ expected_count += 1
+ # Use all new objects - avoids caching issues invalidating the gathered
+ # metrics.
+ login(ADMIN_EMAIL)
+ ppa = self.factory.makeArchive()
+ viewer = self.factory.makePerson(password="test")
+ browser = self.getUserBrowser(user=viewer)
+ with person_logged_in(viewer):
+ for i in range(2):
+ pkg = self.factory.makeSourcePackagePublishingHistory(
+ archive=ppa)
+ self.factory.makeSourcePackagePublishingHistory(archive=ppa,
+ distroseries=pkg.distroseries)
+ url = canonical_url(ppa) + "/+packages"
+ browser.open(url)
+ self.assertThat(collector, HasQueryCount(LessThan(expected_count)))
+
+ def test_binary_query_counts(self):
+ query_baseline = 26
+ # Get the baseline.
+ ppa = self.factory.makeArchive()
+ collector = QueryCollector()
+ collector.register()
+ self.addCleanup(collector.unregister)
+ ppa = self.factory.makeArchive()
+ viewer = self.factory.makePerson(password="test")
+ browser = self.getUserBrowser(user=viewer)
+ with person_logged_in(viewer):
+ # The baseline has one package, because otherwise the short-circuit
+ # prevents the packages iteration happening at all and we're not
+ # actually measuring scaling appropriately.
+ self.factory.makeBinaryPackagePublishingHistory(archive=ppa)
+ url = canonical_url(ppa) + "/+packages"
+ browser.open(url)
+ self.assertThat(collector, HasQueryCount(
+ MatchesAny(LessThan(query_baseline), Equals(query_baseline))))
+ expected_count = collector.count
+ # Use all new objects - avoids caching issues invalidating the gathered
+ # metrics.
+ login(ADMIN_EMAIL)
+ ppa = self.factory.makeArchive()
+ viewer = self.factory.makePerson(password="test")
+ browser = self.getUserBrowser(user=viewer)
+ with person_logged_in(viewer):
+ for i in range(2):
+ pkg = self.factory.makeBinaryPackagePublishingHistory(
+ archive=ppa)
+ self.factory.makeBinaryPackagePublishingHistory(archive=ppa,
+ distroarchseries=pkg.distroarchseries)
+ url = canonical_url(ppa) + "/+packages"
+ browser.open(url)
+ self.assertThat(collector, HasQueryCount(
+ MatchesAny(Equals(expected_count), LessThan(expected_count))))
=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
--- lib/lp/soyuz/interfaces/publishing.py 2010-11-07 22:51:53 +0000
+++ lib/lp/soyuz/interfaces/publishing.py 2010-11-10 04:28:49 +0000
@@ -1079,8 +1079,7 @@
`IBinaryPackagePublishingHistory`.
"""
- def getBuildStatusSummariesForSourceIdsAndArchive(source_ids, archive,
- get_builds=None):
+ def getBuildStatusSummariesForSourceIdsAndArchive(source_ids, archive):
"""Return a summary of the build statuses for source publishing ids.
This method collects all the builds for the provided source package
@@ -1095,9 +1094,6 @@
:param archive: The archive which will be used to filter the source
ids.
:type archive: `IArchive`
- :param get_builds: Optional override to replace database querying for
- build records. This is used in the ArchivePublication adapter code
- path and should be avoided where possible.
:return: A dict consisting of the overall status summaries for the
given ids that belong in the archive. For example:
{
@@ -1108,8 +1104,7 @@
:rtype: ``dict``.
"""
- def getBuildStatusSummaryForSourcePublication(source_publication,
- get_builds=None):
+ def getBuildStatusSummaryForSourcePublication(source_publication):
"""Return a summary of the build statuses for this source
publication.
@@ -1117,13 +1112,6 @@
for details. The call is just proxied here so that it can also be
used with an ArchiveSourcePublication passed in as
the source_package_pub, allowing the use of the cached results.
-
- :param get_builds: An optional callback to replace the database lookup
- builds. This is used by some adapters as part of a preloading
- strategy - but the preferred form is to have
- getBuildStatusSummariesForSourceIdsAndArchive (which this call
- delegates to) perform the initial and only loading of the build
- records.
"""
def getNearestAncestor(
=== modified file 'lib/lp/soyuz/interfaces/sourcepackagerelease.py'
--- lib/lp/soyuz/interfaces/sourcepackagerelease.py 2010-08-25 00:29:36 +0000
+++ lib/lp/soyuz/interfaces/sourcepackagerelease.py 2010-11-10 04:28:49 +0000
@@ -98,6 +98,7 @@
files = Attribute("IBinaryPackageFile entries for this "
"sourcepackagerelease")
sourcepackagename = Attribute("SourcePackageName table reference")
+ sourcepackagenameID = Attribute("SourcePackageName id.")
upload_distroseries = Attribute("The distroseries in which this package "
"was first uploaded in Launchpad")
publishings = Attribute("MultipleJoin on SourcepackagePublishing")
=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py 2010-11-09 00:01:48 +0000
+++ lib/lp/soyuz/model/publishing.py 2010-11-10 04:28:49 +0000
@@ -1699,41 +1699,32 @@
return result_set.one()
def getBuildStatusSummariesForSourceIdsAndArchive(self, source_ids,
- archive, get_builds=None):
+ archive):
"""See `IPublishingSet`."""
# source_ids can be None or an empty sequence.
if not source_ids:
return {}
store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
- if len(source_ids) == 1 and get_builds is not None:
- # Fake out the DB access for ArchivePublication
- source_pubs = list(store.find(
+ build_info = list(
+ self.getBuildsForSourceIds(source_ids, archive=archive))
+ source_pubs = set()
+ found_source_ids = set()
+ for row in build_info:
+ source_pubs.add(row[0])
+ found_source_ids.add(row[0].id)
+ pubs_without_builds = set(source_ids) - found_source_ids
+ if pubs_without_builds:
+ # Add in source pubs for which no builds were found: we may in
+ # future want to make this a LEFT OUTER JOIN in
+ # getBuildsForSourceIds but to avoid destabilising other code
+ # paths while we fix performance, it is just done as a single
+ # separate query for now.
+ source_pubs.update(store.find(
SourcePackagePublishingHistory,
- SourcePackagePublishingHistory.id.is_in(source_ids),
+ SourcePackagePublishingHistory.id.is_in(
+ pubs_without_builds),
SourcePackagePublishingHistory.archive == archive))
- source = source_pubs[0]
- build_info = [(source, build, None) for build in get_builds()]
- else:
- build_info = list(
- self.getBuildsForSourceIds(source_ids, archive=archive))
- source_pubs = set()
- found_source_ids = set()
- for row in build_info:
- source_pubs.add(row[0])
- found_source_ids.add(row[0].id)
- pubs_without_builds = set(source_ids) - found_source_ids
- if pubs_without_builds:
- # Add in source pubs for which no builds were found: we may in
- # future want to make this a LEFT OUTER JOIN in
- # getBuildsForSourceIds but to avoid destabilising other code
- # paths while we fix performance, it is just done as a single
- # separate query for now.
- source_pubs.update(store.find(
- SourcePackagePublishingHistory,
- SourcePackagePublishingHistory.id.is_in(
- pubs_without_builds),
- SourcePackagePublishingHistory.archive == archive))
# For each source_pub found, provide an aggregate summary of its
# builds.
binarypackages = getUtility(IBinaryPackageBuildSet)
@@ -1774,8 +1765,7 @@
return source_build_statuses
- def getBuildStatusSummaryForSourcePublication(self, source_publication,
- get_builds=None):
+ def getBuildStatusSummaryForSourcePublication(self, source_publication):
"""See `ISourcePackagePublishingHistory`.getStatusSummaryForBuilds.
This is provided here so it can be used by both the SPPH as well
@@ -1785,7 +1775,7 @@
"""
source_id = source_publication.id
return self.getBuildStatusSummariesForSourceIdsAndArchive([source_id],
- source_publication.archive, get_builds=get_builds)[source_id]
+ source_publication.archive)[source_id]
def requestDeletion(self, sources, removed_by, removal_comment=None):
"""See `IPublishingSet`."""
