launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-completed-jobs-for-celery into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1019215 in Launchpad itself: "InvalidTransition: Transition from Completed to Running is invalid."
  https://bugs.launchpad.net/launchpad/+bug/1019215

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/no-completed-jobs-for-celery/+merge/168859

Skip jobs in BaseJobRunner.runJob() that are completed. We log them, add them to the completed tally and move on.

Also perform some cleanup, such as no longer exporting JobStatus in model.job, whereas it's actually defined in interfaces.job, and only one module was pulling it from model.job anyway.
-- 
https://code.launchpad.net/~stevenk/launchpad/no-completed-jobs-for-celery/+merge/168859
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/no-completed-jobs-for-celery into lp:launchpad.

=== modified file 'lib/lp/code/scripts/tests/test_scan_branches.py'
--- lib/lp/code/scripts/tests/test_scan_branches.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/scripts/tests/test_scan_branches.py	2013-06-12 06:56:37 +0000
@@ -1,6 +1,4 @@
-#! /usr/bin/python
-#
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test the scan_branches script."""
@@ -19,10 +17,8 @@
     BranchJobType,
     BranchScanJob,
     )
-from lp.services.job.model.job import (
-    Job,
-    JobStatus,
-    )
+from lp.services.job.interfaces.job import JobStatus
+from lp.services.job.model.job import Job
 from lp.services.osutils import override_environ
 from lp.services.scripts.tests import run_script
 from lp.testing import TestCaseWithFactory

=== modified file 'lib/lp/services/job/model/job.py'
--- lib/lp/services/job/model/job.py	2012-06-14 05:18:22 +0000
+++ lib/lp/services/job/model/job.py	2013-06-12 06:56:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 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).
 
 """ORM object representing jobs."""
@@ -8,7 +8,6 @@
     'EnumeratedSubclass',
     'InvalidTransition',
     'Job',
-    'JobStatus',
     'UniversalJobSource',
     ]
 

=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py	2013-05-15 04:41:33 +0000
+++ lib/lp/services/job/runner.py	2013-06-12 06:56:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 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).
 
 """Facilities for running Jobs."""
@@ -73,6 +73,7 @@
 from lp.services.job.interfaces.job import (
     IJob,
     IRunnableJob,
+    JobStatus,
     )
 from lp.services.mail.sendmail import (
     MailController,
@@ -291,6 +292,10 @@
         return True
 
     def runJob(self, job, fallback):
+        if job.status == JobStatus.COMPLETED:
+            self.logger.debug('Skipping completed job %s' % self.job_str(job))
+            self.completed_jobs.append(job)
+            return None
         super(BaseJobRunner, self).runJob(IRunnableJob(job), fallback)
 
     def userErrorTypes(self, job):

=== modified file 'lib/lp/services/job/tests/test_runner.py'
--- lib/lp/services/job/tests/test_runner.py	2012-07-26 14:31:45 +0000
+++ lib/lp/services/job/tests/test_runner.py	2013-06-12 06:56:37 +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).
 
 """Tests for job-running facilities."""
@@ -17,6 +17,7 @@
 from testtools.testcase import ExpectedException
 import transaction
 from zope.interface import implements
+from zope.security.proxy import removeSecurityProxy
 
 from lp.code.interfaces.branchmergeproposal import IUpdatePreviewDiffJobSource
 from lp.services.config import config
@@ -377,6 +378,13 @@
         self.assertNotIn(job, runner.completed_jobs)
         self.assertIn(job, runner.incomplete_jobs)
 
+    def test_runJob_does_not_run_completed_job(self):
+        job_1 = NullJob('job 1')
+        removeSecurityProxy(job_1.job)._status = JobStatus.COMPLETED
+        runner = JobRunner(job_1)
+        runner.runJob(job_1, None)
+        self.assertEqual([job_1], runner.completed_jobs)
+
     def test_taskId(self):
         # BaseRunnableJob.taskId() creates a task ID that consists
         # of the Job's class name, the job ID and a UUID.