Merge lp:~jcsackett/launchpad/archivemirror-timeout-618400 into lp:launchpad

[j.c.sackett]

[r=lifeless][ui=none][bug=618400][incr] Provides a method to preload countries when pulling mirrors by country to remove a timeout on +archivemirrors

Summary
=======

Distribution:+archivemirrors has a timeout caused by a combination of factors: 1) repeated country queries for each distributionmirror that get expensive in aggregate and 2) a very expensive mirror freshness query/calculation.

This branch addresses the first cause; a second branch will need to be landed to address the second cause and close bug 618400.

In the process there is a slight refactor to remove some repeated code.

Proposed fix
============

When getting distribution mirrors, get the countries for the mirrors at the same time, rather than querying for them one by one later.

Preimplementation
=================

Spoke with Curtis Hovey and William Grant about the country query issue.

Implementation
==============

lib/lp/registry/interfaces/distribution.py
lib/lp/registry/model/distribution.py
-------------------------------------

Added a method, _getActiveMirrors, which can generate the resultset for either cdimages or archive mirrors, with or without the country join. Updated the archive_mirrors and cdimage_mirrors properties to use this method. Added an archive_mirrors_by_country and cdimage_mirrors_by_country property that also uses this method, and added them as not-exported CollectionFields to the interface.

lib/lp/registry/browser/distribution.py
---------------------------------------

Updated the +archivemirrors view and +cdmirrors view to use their respective _by_country property. Updated the parent view to use the returned data properly.

lib/lp/registry/model/distributionmirror.py
-------------------------------------------

Drive by reformat of the __all__ clause per style guidelines.

lib/lp/registry/browser/tests/distribution-views.txt
----------------------------------------------------

Updated test to include archive mirrors alongside cdimage.

Tests
=====

bin/test -t distribution-views\.txt
bin/test -t distributionmirror-views\.txt

QA
==

Make certain that Distribution:+archivemirrors loads fine. Then, make certain it doesn't timeout and ceases to be in the timeout reports. The bug itself doesn't indicate how often the timeout occurs, which makes this a touch difficult to QA.

Lint
====

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/distribution.py
  lib/lp/registry/browser/tests/distribution-views.txt
  lib/lp/registry/interfaces/distribution.py
  lib/lp/registry/model/distribution.py
  lib/lp/registry/model/distributionmirror.py

./lib/lp/registry/model/distributionmirror.py
     594: E501 line too long (80 characters)
     460: Line exceeds 78 characters.
     478: Line exceeds 78 characters.
     594: Line exceeds 78 characters.
     812: Line exceeds 78 characters.
     865: Line exceeds 78 characters.

The above lint errors are from a number of queries (not in this branch) that I think are already hard enough to read without odd wrapping changes.

[Robert Collins (lifeless)]

A few small comments.

Firstly, rather than selecting wider, consider using the pre_iter_hook on DecoratedResultSet.

This trades off redundant data for a second query with less duplication. It's usually a win.

That said, because you're not exposing the ResultSet we don't even need DecoratedResultSet - so we can just do this:

+ 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,
+ )))
+ if not by_country or not mirrors:
+ return mirrors
+ else:
+ list(Store.of(self).find(Country, Country.id.is_in(
+ [mirror.countryID for mirror in mirrors]))

Secondly, the new collections probably want doNotSnapshot on them.

Cheers,
Rob

review: Approve

[j.c.sackett (jcsackett)]

Rob--

I started making the larger change mentioned above, and I have run into some confusion.

With the list of Countries created in the last line of the diff you provided, I don't see a way to reconcile mirror with country in the form we need down the road, without loading mirror.country in some way to match; I suppose one could bounce around with Country.id and mirror.countryID to get the pairs matched up, but it seems odd.

It occured to me though that perhaps there's some Storm magic here I don't understand; does the process of querying for the countries above lead to the country attribute on DistributionMirror already being populated, so I could just use mirror.country without causing a query down the chain?

=== modified file 'lib/lp/registry/browser/distribution.py'
--- lib/lp/registry/browser/distribution.py	2010-12-30 12:50:16 +0000
+++ lib/lp/registry/browser/distribution.py	2011-01-19 15:54:00 +0000
@@ -980,7 +980,11 @@
 
     @cachedproperty
     def mirrors(self):
-        return self.context.archive_mirrors
+        return self.context.archive_mirrors_by_country
+
+    @cachedproperty
+    def mirror_count(self):
+        return len(self.mirrors)
 
 
 class DistributionSeriesMirrorsView(DistributionMirrorsView):
@@ -992,8 +996,11 @@
 
     @cachedproperty
     def mirrors(self):
-        return self.context.cdimage_mirrors
+        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/distribution-views.txt'
--- lib/lp/registry/browser/tests/distribution-views.txt	2010-12-16 19:59:20 +0000
+++ lib/lp/registry/browser/tests/distribution-views.txt	2011-01-19 15:54:00 +0000
@@ -25,6 +25,22 @@
     Germany: 1 mirror(s)
     United Kingdom: 1 mirror(s)
 
+Lists of archive mirrors
+------------------------
+
+    >>> ubuntu_archivemirrors = create_view(ubuntu, name="+archivemirrors")
+    >>> country_and_mirrors = (
+    ...     ubuntu_archivemirrors.getMirrorsGroupedByCountry())
+    >>> for country_and_mirror in country_and_mirrors:
+    ...     country = country_and_mirror['country']
+    ...     mirrors = country_and_mirror['mirrors']
+    ...     for mirror in mirrors:
+    ...         assert mirror.country.name == country
+    ...     print "%s: %d mirror(s)" % (country, len(mirrors))
+    Antarctica: 1 mirror(s)
+    France: 2 mirror(s)
+    United Kingdom: 1 mirror(s)
+
 
 Distribution modification views
 ===============================

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2010-12-31 03:39:53 +0000
+++ lib/lp/registry/interfaces/distribution.py	2011-01-19 15:54:00 +0000
@@ -239,11 +239,19 @@
             description=_("All enabled and official ARCHIVE mirrors "
                           "of this Distribution."),
             readonly=True, value_type=Object(schema=IDistributionMirror))))
+    archive_mirrors_by_country = doNotSnapshot(CollectionField(
+            description=_("All enabled and official ARCHIVE mirrors "
+                          "of this Distribution."),
+            readonly=True, value_type=Object(schema=IDistributionMirror)))
     cdimage_mirrors = exported(doNotSnapshot(
         CollectionField(
             description=_("All enabled and official RELEASE mirrors "
                           "of this Distribution."),
             readonly=True, value_type=Object(schema=IDistributionMirror))))
+    cdimage_mirrors_by_country = doNotSnapshot(CollectionField(
+            description=_("All enabled and official ARCHIVE mirrors "
+                          "of this Distribution."),
+            readonly=True, value_type=Object(schema=IDistributionMirror)))
     disabled_mirrors = Attribute(
         "All disabled and official mirrors of this Distribution.")
     unofficial_mirrors = Attribute(

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2010-12-21 00:26:41 +0000
+++ lib/lp/registry/model/distribution.py	2011-01-19 15:54:00 +0000
@@ -19,6 +19,7 @@
     )
 from sqlobject.sqlbuilder import SQLConstant
 from storm.locals import (
+    And,
     Desc,
     Int,
     Join,
@@ -155,6 +156,7 @@
     cachedproperty,
     get_property_cache,
     )
+from lp.services.worlddata.model.country import Country
 from lp.soyuz.enums import (
     ArchivePurpose,
     ArchiveStatus,
@@ -439,27 +441,46 @@
         else:
             return [archive.id]
 
+    def _getActiveMirrors(self, mirror_content_type, by_country=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)))
+        if by_country and mirrors:
+            #Since country data is needed, fetch countries into the cache.
+            countries = list(Store.of(self).find(
+                Country,
+                Country.id.is_in(mirror.countryID for mirror in mirrors)))
+        return mirrors
+
     @property
     def archive_mirrors(self):
         """See `IDistribution`."""
-        return Store.of(self).find(
-            DistributionMirror,
-            distribution=self,
-            content=MirrorContent.ARCHIVE,
-            enabled=True,
-            status=MirrorStatus.OFFICIAL,
-            official_candidate=True)
-
-    @property
-    def cdimage_mirrors(self):
-        """See `IDistribution`."""
-        return Store.of(self).find(
-            DistributionMirror,
-            distribution=self,
-            content=MirrorContent.RELEASE,
-            enabled=True,
-            status=MirrorStatus.OFFICIAL,
-            official_candidate=True)
+        return self._getActiveMirrors(MirrorContent.ARCHIVE)
+
+    @property
+    def archive_mirrors_by_country(self):
+        """See `IDistribution`."""
+        return self._getActiveMirrors(
+            MirrorContent.ARCHIVE,
+            by_country=True)
+
+    @property
+    def cdimage_mirrors(self, by_country=False):
+        """See `IDistribution`."""
+        return self._getActiveMirrors(MirrorContent.RELEASE)
+
+    @property
+    def cdimage_mirrors_by_country(self):
+        """See `IDistribution`."""
+        return self._getActiveMirrors(
+            MirrorContent.RELEASE,
+            by_country=True)
 
     @property
     def disabled_mirrors(self):

=== modified file 'lib/lp/registry/model/distributionmirror.py'
--- lib/lp/registry/model/distributionmirror.py	2010-09-29 14:38:30 +0000
+++ lib/lp/registry/model/distributionmirror.py	2011-01-19 15:54:00 +0000
@@ -6,9 +6,14 @@
 """Module docstring goes here."""
 
 __metaclass__ = type
-__all__ = ['DistributionMirror', 'MirrorDistroArchSeries',
-           'MirrorDistroSeriesSource', 'MirrorProbeRecord',
-           'DistributionMirrorSet', 'MirrorCDImageDistroSeries']
+__all__ = [
+    'DistributionMirror',
+    'MirrorDistroArchSeries',
+    'MirrorDistroSeriesSource',
+    'MirrorProbeRecord',
+    'DistributionMirrorSet',
+    'MirrorCDImageDistroSeries',
+    ]
 
 from datetime import (
     datetime,