launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-824553 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #824553 in Launchpad itself: "publish-ftpmaster script runs publisher too many times"
  https://bugs.launchpad.net/launchpad/+bug/824553

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-824553/+merge/71823

= Summary =

When publish-ftpmaster perceives a new distroseries, instead of doing its normal work it will create the series' initial indexes.

It does that with one separate publish-distro script run per suite.  This is wasteful: a lot of work is repeated for every suite that needs to be done only once.


== Proposed fix ==

Instead, pass the full list of suites to publish-distro.  It accepts multiple instances of the -s <suite> argument.


== Pre-implementation notes ==

I neglected to have a pre-implementation call about this.  The bug specified what needed doing with great clarity and in sufficient detail.


== Implementation details ==

This may mean more repeated work if initial indexing should fail and have to be re-done on the next run.  But why optimize for failure?


== Tests ==

{{{
./bin/test -vvc lp.archivepublisher.tests.test_publish_ftpmaster
}}}


== Demo and Q/A ==

Create a new Ubuntu distroseries (STOP!  I MEANT ON A TEST SERVER, YOU FOOL!) and run publish-ftpmaster with -d ubuntu.  It'll churn for a while.  In the Ubuntu archive (e.g. /srv/launchpad.net/ubuntu-archive/ubuntu) you'll see marker files named ".created-indexes-for-<series><pocket-suffix> for each of its pockets.

Well, you probably won't.  But if you had remembered to "ls -a" then you almost definitely would.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/scripts/publish_ftpmaster.py
  lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
-- 
https://code.launchpad.net/~jtv/launchpad/bug-824553/+merge/71823
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-824553 into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-08-15 10:50:57 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-08-17 07:50:25 +0000
@@ -295,11 +295,13 @@
                 "Indexes for %s were created on %s.\n"
                 % (suite, datetime.now(utc)))
 
-    def createIndexes(self, distribution, suite):
-        """Create archive indexes for `distroseries`."""
-        self.logger.info("Creating archive indexes for %s.", suite)
-        self.runPublishDistro(distribution, args=['-A'], suites=[suite])
-        self.markIndexCreationComplete(distribution, suite)
+    def createIndexes(self, distribution, suites):
+        """Create archive indexes for `suites` of `distroseries`."""
+        self.logger.info(
+            "Creating archive indexes for %s.", ', '.join(suites))
+        self.runPublishDistro(distribution, args=['-A'], suites=suites)
+        for suite in suites:
+            self.markIndexCreationComplete(distribution, suite)
 
     def processAccepted(self, distribution):
         """Run the process-accepted script."""
@@ -573,8 +575,7 @@
         for series in distribution.series:
             suites_needing_indexes = self.listSuitesNeedingIndexes(series)
             if len(suites_needing_indexes) > 0:
-                for suite in suites_needing_indexes:
-                    self.createIndexes(distribution, suite)
+                self.createIndexes(distribution, suites_needing_indexes)
                 # Don't try to do too much in one run.  Leave the rest
                 # of the work for next time.
                 return

=== modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
--- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-08-15 23:25:16 +0000
+++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-08-17 07:50:25 +0000
@@ -931,7 +931,7 @@
         script.markIndexCreationComplete = FakeMethod()
         script.runPublishDistro = FakeMethod()
         suite = get_a_suite(series)
-        script.createIndexes(distro, suite)
+        script.createIndexes(distro, [suite])
         self.assertEqual(
             [((distro, suite), {})], script.markIndexCreationComplete.calls)
 
@@ -946,7 +946,7 @@
         script.markIndexCreationComplete = FakeMethod()
         script.runPublishDistro = FakeMethod(failure=Boom("Sorry!"))
         try:
-            script.createIndexes(series.distribution, get_a_suite(series))
+            script.createIndexes(series.distribution, [get_a_suite(series)])
         except:
             pass
         self.assertEqual([], script.markIndexCreationComplete.calls)
@@ -999,16 +999,18 @@
 
     def test_script_calls_createIndexes_for_new_series(self):
         # If the script's main() finds a distroseries that needs its
-        # indexes created, it calls createIndexes on that distroseries.
+        # indexes created, it calls createIndexes on that distroseries,
+        # passing it all of the series' suite names.
         distro = self.makeDistroWithPublishDirectory()
         series = self.makeDistroSeriesNeedingIndexes(distribution=distro)
         script = self.makeScript(distro)
         script.createIndexes = FakeMethod()
         script.main()
-        expected_calls = [
-            ((distro, series.getSuite(pocket)), {})
-            for pocket in pocketsuffix.iterkeys()]
-        self.assertContentEqual(expected_calls, script.createIndexes.calls)
+        [((given_distro, given_suites), kwargs)] = script.createIndexes.calls
+        self.assertEqual(distro, given_distro)
+        self.assertContentEqual(
+            [series.getSuite(pocket) for pocket in pocketsuffix.iterkeys()],
+            given_suites)
 
     def test_createIndexes_ignores_other_series(self):
         # createIndexes does not accidentally also touch other
@@ -1022,7 +1024,7 @@
         self.createIndexesMarkerDir(script, series)
         suite = get_a_suite(series)
 
-        script.createIndexes(distro, suite)
+        script.createIndexes(distro, [suite])
 
         args, kwargs = script.runPublishDistro.calls[0]
         self.assertEqual([suite], kwargs['suites'])