launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/archivemirror-timeout-618400 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #618400 Distribution:+archivemirrors timing out in 1% of requests
  https://bugs.launchpad.net/bugs/618400

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/archivemirror-timeout-618400/+merge/46550

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.
-- 
https://code.launchpad.net/~jcsackett/launchpad/archivemirror-timeout-618400/+merge/46550
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/archivemirror-timeout-618400 into lp:launchpad.