launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #782096 in Launchpad itself: "PackageCopyJob.getActiveJobs() should check job status."
  https://bugs.launchpad.net/launchpad/+bug/782096

For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-782096/+merge/61612

= Summary =

IPlainPackageCopyJobSource.getActiveJobs() had two flaws that made it a poor way to fetch the next job to execute:

1. It failed to check for job status, so unless finished jobs were immediately deleted, it would pick up completed or failed jobs.

2. It failed to impose a sane ordering, so it wouldn't necessarily pick up the oldest available job.

This fixes that.

The test uses the makeJob helper, which isn't actually in db-devel at the time of writing.  But it's on the way through EC2 and should land soon.  Tests pass when I merge in that branch, but I wanted to keep the diff minimal so I left it out.  (There were no text conflicts to complicate this, luckily).


== Tests ==

{{{
./bin/test -vvc lp.soyuz.tests.test_packagecopyjob
}}}


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/model/packagecopyjob.py
  lib/lp/soyuz/tests/test_packagecopyjob.py
-- 
https://code.launchpad.net/~jtv/launchpad/db-bug-782096/+merge/61612
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-782096 into lp:launchpad/db-devel.

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2011-05-10 09:15:18 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2011-05-19 16:46:16 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -33,6 +33,7 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.distroseries import DistroSeries
 from lp.services.database.stormbase import StormBase
+from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.job.runner import BaseRunnableJob
 from lp.soyuz.interfaces.archive import CannotCopy
@@ -171,7 +172,9 @@
         jobs = IStore(PackageCopyJob).find(
             PackageCopyJob,
             PackageCopyJob.job_type == cls.class_job_type,
-            PackageCopyJob.target_archive == target_archive)
+            PackageCopyJob.target_archive == target_archive,
+            Job.id == PackageCopyJob.job_id,
+            Job._status == JobStatus.WAITING)
         return DecoratedResultSet(jobs, cls)
 
     @property

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-10 09:15:18 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2011-05-19 16:46:16 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for sync package jobs."""
@@ -10,6 +10,7 @@
 
 from canonical.testing import LaunchpadZopelessLayer
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.job.interfaces.job import JobStatus
 from lp.soyuz.interfaces.archive import CannotCopy
 from lp.soyuz.interfaces.packagecopyjob import (
     IPackageCopyJob,
@@ -65,6 +66,21 @@
             include_binaries=False)
         self.assertContentEqual([job], source.getActiveJobs(archive2))
 
+    def test_getActiveJobs_gets_oldest_first(self):
+        # getActiveJobs returns the oldest available job first.
+        dsd = self.factory.makeDistroSeriesDifference()
+        target_archive = dsd.derived_series.main_archive
+        jobs = [self.makeJob(dsd) for counter in xrange(2)]
+        source = getUtility(IPlainPackageCopyJobSource)
+        self.assertEqual(jobs[0], source.getActiveJobs(target_archive)[0])
+
+    def test_getActiveJobs_only_returns_waiting_jobs(self):
+        # getActiveJobs ignores jobs that aren't in the WAITING state.
+        job = self.makeJob(self.factory.makeDistroSeriesDifference())
+        removeSecurityProxy(job).job._status = JobStatus.RUNNING
+        source = getUtility(IPlainPackageCopyJobSource)
+        self.assertContentEqual([], source.getActiveJobs(job.target_archive))
+
     def test_run_unknown_package(self):
         # A job properly records failure.
         distroseries = self.factory.makeDistroSeries()