launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17893
[Merge] lp:~cjwatson/launchpad/mirror-preload into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/mirror-preload into lp:launchpad.
Commit message:
Ensure that the various mirror views have constant query counts in the number of mirrors and countries involved.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1415447 in Launchpad itself: "Distribution:+disabledmirrors times out"
https://bugs.launchpad.net/launchpad/+bug/1415447
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/mirror-preload/+merge/250185
Only some of the various mirror views were query-optimised. This takes care of the rest. I had to turn DistributionMirror.cdimage_series into a cachedproperty, which entailed a few changes in various places to cope with it returning a list rather than a ResultSet.
A TAL subtlety I hadn't previously known about: <element tal:condition="cond" tal:define="key context/method" /> will evaluate context.method() even if cond is False. To avoid the evaluation, we must use an additional nested element.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/mirror-preload into lp:launchpad.
=== modified file 'lib/lp/registry/browser/distribution.py'
--- lib/lp/registry/browser/distribution.py 2014-10-22 19:25:39 +0000
+++ lib/lp/registry/browser/distribution.py 2015-02-18 18:49:05 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2014 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Browser views for distributions."""
@@ -1104,7 +1104,7 @@
@cachedproperty
def mirror_count(self):
- return self.mirrors.count()
+ return len(self.mirrors)
def _sum_throughput(self, mirrors):
"""Given a list of mirrors, calculate the total bandwidth
@@ -1181,10 +1181,6 @@
def mirrors(self):
return self.context.archive_mirrors_by_country
- @cachedproperty
- def mirror_count(self):
- return len(self.mirrors)
-
class DistributionSeriesMirrorsView(DistributionMirrorsView):
@@ -1197,10 +1193,6 @@
def mirrors(self):
return self.context.cdimage_mirrors_by_country
- @cachedproperty
- def mirror_count(self):
- return len(self.mirrors)
-
class DistributionMirrorsRSSBaseView(LaunchpadView):
"""A base class for RSS feeds of distribution mirrors."""
=== modified file 'lib/lp/registry/browser/tests/test_distribution_views.py'
--- lib/lp/registry/browser/tests/test_distribution_views.py 2014-08-01 06:04:58 +0000
+++ lib/lp/registry/browser/tests/test_distribution_views.py 2015-02-18 18:49:05 +0000
@@ -1,23 +1,34 @@
-# Copyright 2009-2014 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
import soupmatchers
-from testtools.matchers import MatchesStructure
+from testtools.matchers import (
+ Equals,
+ MatchesStructure,
+ )
from zope.component import getUtility
from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
from lp.registry.browser.distribution import DistributionPublisherConfigView
from lp.registry.interfaces.distribution import IDistributionSet
+from lp.registry.interfaces.distributionmirror import (
+ MirrorContent,
+ MirrorStatus,
+ )
from lp.services.webapp.servers import LaunchpadTestRequest
+from lp.services.worlddata.interfaces.country import ICountrySet
from lp.soyuz.interfaces.processor import IProcessorSet
from lp.testing import (
login,
login_celebrity,
+ login_person,
+ record_two_runs,
TestCaseWithFactory,
)
from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
from lp.testing.sampledata import LAUNCHPAD_ADMIN
from lp.testing.views import create_initialized_view
@@ -333,3 +344,84 @@
'Header should say maintainer (not owner)', 'h1',
text='Change the maintainer of Boobuntu'))
self.assertThat(view.render(), header_match)
+
+
+class TestDistributionMirrorsViewMixin:
+ """Mixin to help test a distribution mirrors view."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_query_count(self):
+ # The number of queries required to render the mirror table is
+ # constant in the number of mirrors.
+ person = self.factory.makePerson()
+ distro = self.factory.makeDistribution(owner=person)
+ login_celebrity("admin")
+ distro.supports_mirrors = True
+ login_person(person)
+ distro.mirror_admin = person
+ countries = iter(getUtility(ICountrySet))
+
+ def render_mirrors():
+ text = create_initialized_view(
+ distro, self.view, principal=person).render()
+ self.assertNotIn("We don't know of any", text)
+ return text
+
+ def create_mirror():
+ mirror = self.factory.makeMirror(
+ distro, country=next(countries), official_candidate=True)
+ self.configureMirror(mirror)
+
+ recorder1, recorder2 = record_two_runs(
+ render_mirrors, create_mirror, 10)
+ self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+
+
+class TestDistributionArchiveMirrorsView(
+ TestDistributionMirrorsViewMixin, TestCaseWithFactory):
+
+ view = "+archivemirrors"
+
+ def configureMirror(self, mirror):
+ mirror.enabled = True
+ mirror.status = MirrorStatus.OFFICIAL
+
+
+class TestDistributionSeriesMirrorsView(
+ TestDistributionMirrorsViewMixin, TestCaseWithFactory):
+
+ view = "+cdmirrors"
+
+ def configureMirror(self, mirror):
+ mirror.enabled = True
+ mirror.content = MirrorContent.RELEASE
+ mirror.status = MirrorStatus.OFFICIAL
+
+
+class TestDistributionDisabledMirrorsView(
+ TestDistributionMirrorsViewMixin, TestCaseWithFactory):
+
+ view = "+disabledmirrors"
+
+ def configureMirror(self, mirror):
+ mirror.enabled = False
+ mirror.status = MirrorStatus.OFFICIAL
+
+
+class TestDistributionUnofficialMirrorsView(
+ TestDistributionMirrorsViewMixin, TestCaseWithFactory):
+
+ view = "+unofficialmirrors"
+
+ def configureMirror(self, mirror):
+ mirror.status = MirrorStatus.UNOFFICIAL
+
+
+class TestDistributionPendingReviewMirrorsView(
+ TestDistributionMirrorsViewMixin, TestCaseWithFactory):
+
+ view = "+pendingreviewmirrors"
+
+ def configureMirror(self, mirror):
+ mirror.status = MirrorStatus.PENDING_REVIEW
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2015-02-13 10:43:03 +0000
+++ lib/lp/registry/model/distribution.py 2015-02-18 18:49:05 +0000
@@ -9,6 +9,7 @@
'DistributionSet',
]
+from collections import defaultdict
import itertools
from operator import itemgetter
@@ -113,6 +114,7 @@
from lp.registry.model.announcement import MakesAnnouncements
from lp.registry.model.distributionmirror import (
DistributionMirror,
+ MirrorCDImageDistroSeries,
MirrorDistroArchSeries,
MirrorDistroSeriesSource,
)
@@ -130,6 +132,7 @@
from lp.registry.model.oopsreferences import referenced_oops
from lp.registry.model.pillar import HasAliasMixin
from lp.registry.model.sourcepackagename import SourcePackageName
+from lp.services.database.bulk import load_referencing
from lp.services.database.constants import UTC_NOW
from lp.services.database.datetimecol import UtcDateTimeCol
from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -422,17 +425,21 @@
else:
return [archive.id]
- def _getActiveMirrors(self, mirror_content_type,
- by_country=False, needs_fresh=False):
+ def _getMirrors(self, content=None, enabled=True,
+ status=MirrorStatus.OFFICIAL, by_country=False,
+ needs_fresh=False, needs_cdimage_series=False):
"""Builds the query to get the mirror data for various purposes."""
- mirrors = list(Store.of(self).find(
- DistributionMirror,
- And(
- DistributionMirror.distribution == self.id,
- DistributionMirror.content == mirror_content_type,
- DistributionMirror.enabled == True,
- DistributionMirror.status == MirrorStatus.OFFICIAL,
- DistributionMirror.official_candidate == True)))
+ clauses = [
+ DistributionMirror.distribution == self.id,
+ DistributionMirror.status == status,
+ ]
+ if content is not None:
+ clauses.append(DistributionMirror.content == content)
+ if enabled is not None:
+ clauses.append(DistributionMirror.enabled == enabled)
+ if status != MirrorStatus.UNOFFICIAL:
+ clauses.append(DistributionMirror.official_candidate == True)
+ mirrors = list(Store.of(self).find(DistributionMirror, And(*clauses)))
if by_country and mirrors:
# Since country data is needed, fetch countries into the cache.
@@ -472,59 +479,71 @@
mirror.id, None)
cache.source_mirror_freshness = source_mirror_freshness.get(
mirror.id, None)
+
+ if needs_cdimage_series and mirrors:
+ all_cdimage_series = load_referencing(
+ MirrorCDImageDistroSeries, mirrors, ["distribution_mirrorID"])
+ cdimage_series = defaultdict(list)
+ for series in all_cdimage_series:
+ cdimage_series[series.distribution_mirrorID].append(series)
+ for mirror in mirrors:
+ cache = get_property_cache(mirror)
+ cache.cdimage_series = cdimage_series.get(mirror.id, [])
+
return mirrors
@property
def archive_mirrors(self):
"""See `IDistribution`."""
- return self._getActiveMirrors(MirrorContent.ARCHIVE)
+ return self._getMirrors(content=MirrorContent.ARCHIVE)
@property
def archive_mirrors_by_country(self):
"""See `IDistribution`."""
- return self._getActiveMirrors(
- MirrorContent.ARCHIVE,
+ return self._getMirrors(
+ content=MirrorContent.ARCHIVE,
by_country=True,
needs_fresh=True)
@property
def cdimage_mirrors(self, by_country=False):
"""See `IDistribution`."""
- return self._getActiveMirrors(MirrorContent.RELEASE)
+ return self._getMirrors(
+ content=MirrorContent.RELEASE,
+ needs_cdimage_series=True)
@property
def cdimage_mirrors_by_country(self):
"""See `IDistribution`."""
- return self._getActiveMirrors(
- MirrorContent.RELEASE,
- by_country=True)
+ return self._getMirrors(
+ content=MirrorContent.RELEASE,
+ by_country=True,
+ needs_cdimage_series=True)
@property
def disabled_mirrors(self):
"""See `IDistribution`."""
- return Store.of(self).find(
- DistributionMirror,
- distribution=self,
+ return self._getMirrors(
enabled=False,
- status=MirrorStatus.OFFICIAL,
- official_candidate=True)
+ by_country=True,
+ needs_fresh=True)
@property
def unofficial_mirrors(self):
"""See `IDistribution`."""
- return Store.of(self).find(
- DistributionMirror,
- distribution=self,
- status=MirrorStatus.UNOFFICIAL)
+ return self._getMirrors(
+ enabled=None,
+ status=MirrorStatus.UNOFFICIAL,
+ by_country=True,
+ needs_fresh=True)
@property
def pending_review_mirrors(self):
"""See `IDistribution`."""
- return Store.of(self).find(
- DistributionMirror,
- distribution=self,
- status=MirrorStatus.PENDING_REVIEW,
- official_candidate=True)
+ return self._getMirrors(
+ enabled=None,
+ by_country=True,
+ status=MirrorStatus.PENDING_REVIEW)
@property
def drivers(self):
=== modified file 'lib/lp/registry/model/distributionmirror.py'
--- lib/lp/registry/model/distributionmirror.py 2013-06-20 05:50:00 +0000
+++ lib/lp/registry/model/distributionmirror.py 2015-02-18 18:49:05 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Module docstring goes here."""
@@ -82,7 +82,10 @@
format_address,
simple_sendmail,
)
-from lp.services.propertycache import cachedproperty
+from lp.services.propertycache import (
+ cachedproperty,
+ get_property_cache,
+ )
from lp.services.webapp import (
canonical_url,
urlappend,
@@ -339,7 +342,7 @@
'For series mirrors we need to know the '
'expected_file_count in order to tell if it should '
'be disabled or not.')
- if expected_file_count > self.cdimage_series.count():
+ if expected_file_count > len(self.cdimage_series):
return True
else:
if not (self.source_series or self.arch_series):
@@ -458,6 +461,7 @@
mirror = MirrorCDImageDistroSeries(
distribution_mirror=self, distroseries=distroseries,
flavour=flavour)
+ del get_property_cache(self).cdimage_series
return mirror
def deleteMirrorCDImageSeries(self, distroseries, flavour):
@@ -467,21 +471,24 @@
flavour=flavour)
if mirror is not None:
mirror.destroySelf()
+ del get_property_cache(self).cdimage_series
def deleteAllMirrorCDImageSeries(self):
"""See IDistributionMirror"""
for mirror in self.cdimage_series:
mirror.destroySelf()
+ del get_property_cache(self).cdimage_series
@property
def arch_series(self):
"""See IDistributionMirror"""
return MirrorDistroArchSeries.selectBy(distribution_mirror=self)
- @property
+ @cachedproperty
def cdimage_series(self):
"""See IDistributionMirror"""
- return MirrorCDImageDistroSeries.selectBy(distribution_mirror=self)
+ return list(
+ MirrorCDImageDistroSeries.selectBy(distribution_mirror=self))
@property
def source_series(self):
=== modified file 'lib/lp/registry/templates/distributionmirror-macros.pt'
--- lib/lp/registry/templates/distributionmirror-macros.pt 2011-07-01 10:33:17 +0000
+++ lib/lp/registry/templates/distributionmirror-macros.pt 2015-02-18 18:49:05 +0000
@@ -46,10 +46,11 @@
<td tal:condition="show_mirror_type">
<span tal:replace="mirror/content/title" />
</td>
- <td tal:condition="show_freshness"
- tal:define="freshness mirror/getOverallFreshness">
- <span tal:content="freshness/title"
- tal:attributes="class string:distromirrorstatus${freshness/name}" />
+ <td tal:condition="show_freshness">
+ <tal:freshness define="freshness mirror/getOverallFreshness">
+ <span tal:content="freshness/title"
+ tal:attributes="class string:distromirrorstatus${freshness/name}" />
+ </tal:freshness>
</td>
</tr>
=== modified file 'lib/lp/registry/tests/test_distributionmirror.py'
--- lib/lp/registry/tests/test_distributionmirror.py 2013-03-12 05:51:28 +0000
+++ lib/lp/registry/tests/test_distributionmirror.py 2015-02-18 18:49:05 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -77,11 +77,9 @@
self.hoary, flavour='ubuntu')
self.cdimage_mirror.ensureMirrorCDImageSeries(
self.hoary, flavour='edubuntu')
- self.failUnlessEqual(
- self.cdimage_mirror.cdimage_series.count(), 2)
+ self.assertEqual(2, len(self.cdimage_mirror.cdimage_series))
self.cdimage_mirror.deleteAllMirrorCDImageSeries()
- self.failUnlessEqual(
- self.cdimage_mirror.cdimage_series.count(), 0)
+ self.assertEqual(0, len(self.cdimage_mirror.cdimage_series))
def test_archive_mirror_without_content_freshness(self):
self.failIf(self.archive_mirror.source_series or
=== modified file 'lib/lp/registry/tests/test_distributionmirror_prober.py'
--- lib/lp/registry/tests/test_distributionmirror_prober.py 2013-05-02 00:40:14 +0000
+++ lib/lp/registry/tests/test_distributionmirror_prober.py 2015-02-18 18:49:05 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""distributionmirror-prober tests."""
@@ -839,11 +839,11 @@
def test_MirrorCDImageSeries_records_are_deleted_before_probing(self):
mirror = DistributionMirror.byName('releases-mirror2')
- self.failUnless(not mirror.cdimage_series.is_empty())
+ self.assertNotEqual(0, len(mirror.cdimage_series))
# Note that calling this function won't actually probe any mirrors; we
# need to call reactor.run() to actually start the probing.
probe_cdimage_mirror(mirror, StringIO(), [], logging)
- self.failUnless(mirror.cdimage_series.is_empty())
+ self.assertEqual(0, len(mirror.cdimage_series))
def test_archive_mirror_probe_function(self):
mirror1 = DistributionMirror.byName('archive-mirror')
Follow ups
