launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/process-job-source-groups-locking into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #839659 in Launchpad itself: "process-job-source-groups.py will starve itself if run with multiple overlapping schedules"
  https://bugs.launchpad.net/launchpad/+bug/839659

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/process-job-source-groups-locking/+merge/117234

== Summary ==

Bug 839659: we can't run process-job-source-groups on overlapping schedules for multiple groups (MAIN and FREQUENT) because it takes a lock whose name doesn't depend on the groups being run.

== Proposed fix ==

It really doesn't need its own lock, as it just runs process-job-source which locks anyway, so drop it.

== Tests ==

bin/test -vvct test_process_job_sources_cronjob

== Demo and Q/A ==

Run 'cronscripts/process-job-source-groups.py MAIN' and 'cronscripts/process-job-source-groups.py FREQUENT' in parallel on e.g. dogfood.  Neither should block on the other, and both should process pending jobs.

== Lint ==

Just the usual false positive for scripts.

./cronscripts/process-job-source-groups.py
      10: '_pythonpath' imported but unused
-- 
https://code.launchpad.net/~cjwatson/launchpad/process-job-source-groups-locking/+merge/117234
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/process-job-source-groups-locking into lp:launchpad.

=== modified file 'cronscripts/process-job-source-groups.py'
--- cronscripts/process-job-source-groups.py	2012-01-01 03:14:54 +0000
+++ cronscripts/process-job-source-groups.py	2012-07-30 10:32:06 +0000
@@ -113,4 +113,6 @@
 
 if __name__ == '__main__':
     script = ProcessJobSourceGroups()
-    script.lock_and_run()
+    # We do not need to take a lock here; all the interesting work is done
+    # by process-job-source.py, which takes its own per-job-source locks.
+    script.run()

=== modified file 'lib/lp/registry/tests/test_process_job_sources_cronjob.py'
--- lib/lp/registry/tests/test_process_job_sources_cronjob.py	2012-06-13 01:03:45 +0000
+++ lib/lp/registry/tests/test_process_job_sources_cronjob.py	2012-07-30 10:32:06 +0000
@@ -102,14 +102,15 @@
         self.assertIn('Group: MAIN\n    I', output)
 
     def test_empty_queue(self):
-        # The script should just create a lockfile, launch a child for
-        # each job source class, and then exit if no jobs are in the queue.
+        # The script should just launch a child for each job source class,
+        # and then exit if no jobs are in the queue.  It should not create
+        # its own lockfile.
         returncode, output, error = run_script(self.script, ['MAIN'])
         expected = (
-            '.*Creating lockfile:.*launchpad-processjobsourcegroups.lock'
             '.*Creating lockfile:.*launchpad-process-job-'
             'source-IMembershipNotificationJobSource.lock.*')
         self.assertTextMatchesExpressionIgnoreWhitespace(expected, error)
+        self.assertNotIn("launchpad-processjobsourcegroups.lock", error)
 
     def test_processed(self):
         # The script should output the number of jobs that have been
@@ -137,8 +138,7 @@
         for source in self.getJobSources("MAIN"):
             args.extend(("--exclude", source))
         returncode, output, error = run_script(self.script, args)
-        expected = "INFO    Creating lockfile: ...\n"
-        self.assertThat(error, DocTestMatches(expected))
+        self.assertEqual("", error)
 
     def test_exclude_non_existing_group(self):
         # If a job source specified by --exclude does not exist the script
@@ -148,7 +148,5 @@
             args.extend(("--exclude", source))
         args.extend(("--exclude", "BobbyDazzler"))
         returncode, output, error = run_script(self.script, args)
-        expected = (
-            "INFO    Creating lockfile: ...\n"
-            "INFO    'BobbyDazzler' is not in MAIN\n")
+        expected = "INFO    'BobbyDazzler' is not in MAIN\n"
         self.assertThat(error, DocTestMatches(expected))