launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/responsive-package-diffs into lp:launchpad.

Commit message:
Drain the package diff queue every time we process it.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1170120 in Launchpad itself: "Package diff generation is unnecessarily unresponsive"
  https://bugs.launchpad.net/launchpad/+bug/1170120

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/responsive-package-diffs/+merge/159515

== Summary ==

Bug 1170120: package diffs are unresponsive and could be faster.

== Proposed fix ==

Make sure to drain the queue every time.  The remaining part of the work will involve a simple lp-production-crontabs change to increase the script frequency.

== Tests ==

bin/test -vvct lp.soyuz.scripts.tests.test_processpendingpackagediffs

== Demo and Q/A ==

Arrange for a couple of package diffs to be requested on dogfood (e.g. by uploads), then run "cronscripts/process-pending-packagediffs.py --batch-size=1" and make sure that it drains the queue.
-- 
https://code.launchpad.net/~cjwatson/launchpad/responsive-package-diffs/+merge/159515
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/responsive-package-diffs into lp:launchpad.

=== modified file 'lib/lp/soyuz/scripts/packagediff.py'
--- lib/lp/soyuz/scripts/packagediff.py	2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/scripts/packagediff.py	2013-04-17 22:23:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """PackageDiff cronscript class."""
@@ -21,12 +21,9 @@
 class ProcessPendingPackageDiffs(LaunchpadCronScript):
 
     def add_my_options(self):
-        # 50 diffs seems to be more them enough to process all uploaded
-        # source packages for 1 hour (average upload rate) for ubuntu
-        # primary archive, security and PPAs in general.
         self.parser.add_option(
-            "-l", "--limit", type="int", default=50,
-            help="Maximum number of requests to be processed in this run.")
+            "--batch-size", type="int", default=50,
+            help="Maximum number of requests to be processed per batch.")
 
         self.parser.add_option(
             "-n", "--dry-run",
@@ -39,24 +36,27 @@
         Collect up to the maximum number of pending `PackageDiff` records
         available and process them.
 
-        Processed diffs results are commited individually.
+        Processed diffs results are committed individually.
         """
         if self.args:
             raise LaunchpadScriptFailure("Unhandled arguments %r" % self.args)
 
         packagediff_set = getUtility(IPackageDiffSet)
 
-        pending_diffs = packagediff_set.getPendingDiffs(
-            limit=self.options.limit)
-        self.logger.debug(
-            'Considering %s diff requests' % pending_diffs.count())
-
-        # Iterate over all pending packagediffs.
-        for packagediff in pending_diffs:
+        while True:
+            pending_diffs = packagediff_set.getPendingDiffs(
+                limit=self.options.batch_size)
+            if pending_diffs.is_empty():
+                break
             self.logger.debug(
-                'Performing package diff for %s from %s' % (
-                    packagediff.from_source.name, packagediff.title))
-            packagediff.performDiff()
-            if not self.options.dryrun:
-                self.logger.debug('Commiting the transaction.')
-                self.txn.commit()
+                'Considering %s diff requests' % pending_diffs.count())
+
+            # Iterate over all pending packagediffs.
+            for packagediff in pending_diffs:
+                self.logger.debug(
+                    'Performing package diff for %s from %s' % (
+                        packagediff.from_source.name, packagediff.title))
+                packagediff.performDiff()
+                if not self.options.dryrun:
+                    self.logger.debug('Committing the transaction.')
+                    self.txn.commit()

=== modified file 'lib/lp/soyuz/scripts/tests/test_processpendingpackagediffs.py'
--- lib/lp/soyuz/scripts/tests/test_processpendingpackagediffs.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/scripts/tests/test_processpendingpackagediffs.py	2013-04-17 22:23:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -19,17 +19,14 @@
     layer = LaunchpadZopelessLayer
     dbuser = config.uploader.dbuser
 
-    def runProcessPendingPackageDiffs(self, extra_args=None):
+    def runProcessPendingPackageDiffs(self):
         """Run process-pending-packagediffs.py.
 
         Returns a tuple of the process's return code, stdout output and
         stderr output."""
-        if extra_args is None:
-            extra_args = []
         script = os.path.join(
             config.root, "cronscripts", "process-pending-packagediffs.py")
         args = [sys.executable, script]
-        args.extend(extra_args)
         process = subprocess.Popen(
             args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
         stdout, stderr = process.communicate()
@@ -47,18 +44,18 @@
         # The pending PackageDiff request was processed.
         self.assertEqual(self.getPendingDiffs().count(), 0)
 
-    def getDiffProcessor(self, limit=None):
+    def getDiffProcessor(self, batch_size=None):
         """Return a `ProcessPendingPackageDiffs` instance.
 
-        :param limit: if passed, it will be used as the 'limit' script
-           argument.
+        :param batch_size: if passed, it will be set as the '--batch-size'
+           script argument.
 
         :return the initialized script object using `BufferLogger` and
            the given parameters.
         """
         test_args = []
-        if limit is not None:
-            test_args.append('-l %s' % limit)
+        if batch_size is not None:
+            test_args.extend(["--batch-size", str(batch_size)])
 
         diff_processor = ProcessPendingPackageDiffs(
             name='process-pending-packagediffs', test_args=test_args)
@@ -80,14 +77,14 @@
         pending_diffs = self.getPendingDiffs()
         self.assertEqual(pending_diffs.count(), 0)
 
-    def testLimitedRun(self):
-        """Run the script with a limited scope.
+    def testDrainsQueue(self):
+        """Run the script with a reduced batch size.
 
-        Check if a limited run of the script only processes up to 'limit'
-        pending diff records and exits.
+        Check that the script drains the queue, even if its batch size is
+        less than the number of pending PackageDiffs.
         """
-        # Setup a DiffProcessor limited to one request per run.
-        diff_processor = self.getDiffProcessor(limit=1)
+        # Setup a DiffProcessor limited to one request per batch.
+        diff_processor = self.getDiffProcessor(batch_size=1)
 
         # Upload a new source version, so we have two pending PackageDiff
         # records to process.
@@ -96,11 +93,10 @@
         self.packager.uploadSourceVersion('1.0-3', suite="warty-updates")
         self.assertEqual(self.getPendingDiffs().count(), 2)
 
-        # The first processor run will process only one PackageDiff,
-        # the other will remain.
-        diff_processor.main()
-        self.assertEqual(self.getPendingDiffs().count(), 1)
-
-        # The next run process the remaining one.
+        # Running the processor drains the queue.
         diff_processor.main()
         self.assertEqual(self.getPendingDiffs().count(), 0)
+
+        # The log output shows two iterations of the processor's main loop.
+        output = diff_processor.logger.getLogBuffer()
+        self.assertEqual(2, output.count("Considering 1 diff request"))