launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02917
[Merge] lp:~jtv/launchpad/bug-730460-job-class into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-730460-job-class into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #730460 in Launchpad itself: "Create DistroSeriesDifferenceJobs"
https://bugs.launchpad.net/launchpad/+bug/730460
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-730460-job-class/+merge/52857
= Summary =
Create a job class DistroSeriesDifferenceJob, and create jobs as needed as source package releases are published.
== Proposed fix ==
It's pathetic, really. I based my new job class on DistributionJob, which isn't really all that similar. The sad, sad fact is that the current job system encourages us to design "class hierarchies" based on what columns a type of job happens to need.
Before I land this branch I may want to implement a feature flag in a follow-up branch, so we don't create lots of DistroSeriesDifferenceJobs too early on. But it'd be nice to have this branch available for integration with other ongoing work.
== Pre-implementation notes ==
Discussed repeatedly with bigjools and StevenK. Using DistributionJob as a base was found to be a relatively practical tradeoff.
Also discussed: the guard code against redundant jobs is fragile, since it relies on a textually exact reproduction of a simple JSON data structure. This wasn't considered a big problem as long as redundant jobs don't dominate the queue.
== Implementation details ==
All fairly straightforward. Surprisingly DistroSeries offered no way to find derived series, so I had to add one.
You'll note that the new job type's "run" method does nothing. StevenK is working on an implementation, but that's a separate task. His branch will probably depend on mine.
== Tests ==
{{{
./bin/test -vvc lp.soyuz.tests.test_distroseriesdifferencejob
./bin/test -vvc lp.registry.tests.test_distroseries
}}}
== Demo and Q/A ==
Source-package publishing must still work, and the appropriate DistroSeriesDifferenceJobs will be created as a side effect.
= Launchpad lint =
I fixed most, but left a few reasonable-looking bits in place:
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/soyuz/interfaces/distroseriesdifferencejob.py
lib/lp/soyuz/model/distroseriesdifferencejob.py
lib/lp/soyuz/model/distributionjob.py
lib/lp/registry/tests/test_distroseries.py
lib/lp/soyuz/model/publishing.py
lib/lp/registry/interfaces/distroseries.py
lib/lp/soyuz/configure.zcml
lib/lp/soyuz/interfaces/distributionjob.py
lib/lp/soyuz/tests/test_distroseriesdifferencejob.py
lib/lp/registry/model/distroseries.py
lib/canonical/launchpad/interfaces/_schema_circular_imports.py
./lib/lp/registry/interfaces/distroseries.py
430: E301 expected 1 blank line, found 2
471: E301 expected 1 blank line, found 0
./lib/lp/registry/model/distroseries.py
403: E301 expected 1 blank line, found 2
--
https://code.launchpad.net/~jtv/launchpad/bug-730460-job-class/+merge/52857
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-730460-job-class into lp:launchpad.
=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2011-03-03 05:12:33 +0000
+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2011-03-10 14:27:03 +0000
@@ -405,6 +405,9 @@
patch_reference_property(IDistroSeries, 'parent_series', IDistroSeries)
patch_plain_parameter_type(
IDistroSeries, 'deriveDistroSeries', 'distribution', IDistribution)
+patch_collection_return_type(
+ IDistroSeries, 'getDerivedSeries', IDistroSeries)
+
# IDistroSeriesDifferenceComment
IDistroSeriesDifferenceComment['comment_author'].schema = IPerson
@@ -552,7 +555,8 @@
# ISpecification
patch_collection_property(ISpecification, 'dependencies', ISpecification)
-patch_collection_property(ISpecification, 'linked_branches', ISpecificationBranch)
+patch_collection_property(
+ ISpecification, 'linked_branches', ISpecificationBranch)
# ISpecificationTarget
patch_entry_return_type(
=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py 2011-03-04 04:28:07 +0000
+++ lib/lp/registry/interfaces/distroseries.py 2011-03-10 14:27:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0211,E0213
@@ -863,6 +863,11 @@
will be.
"""
+ @operation_returns_collection_of(Interface)
+ @export_read_operation()
+ def getDerivedSeries():
+ """Get all `DistroSeries` derived from this one."""
+
class IDistroSeries(IDistroSeriesEditRestricted, IDistroSeriesPublic,
IStructuralSubscriptionTarget):
=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py 2011-03-08 04:43:36 +0000
+++ lib/lp/registry/model/distroseries.py 2011-03-10 14:27:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0611,W0212
@@ -739,10 +739,10 @@
@property
def bugtargetname(self):
"""See IBugTarget."""
- return self.fullseriesname
# XXX mpt 2007-07-10 bugs 113258, 113262:
# The distribution's and series' names should be used instead
# of fullseriesname.
+ return self.fullseriesname
@property
def bugtargetdisplayname(self):
@@ -984,7 +984,7 @@
def getCurrentSourceReleases(self, source_package_names):
"""See `IDistroSeries`."""
return getUtility(IDistroSeriesSet).getCurrentSourceReleases(
- {self:source_package_names})
+ {self: source_package_names})
def getTranslatableSourcePackages(self):
"""See `IDistroSeries`."""
@@ -1928,6 +1928,11 @@
getUtility(IInitialiseDistroSeriesJobSource).create(
child, architectures, packagesets, rebuild)
+ def getDerivedSeries(self):
+ """See `IDistroSeriesPublic`."""
+ return Store.of(self).find(
+ DistroSeries, DistroSeries.parent_series == self)
+
class DistroSeriesSet:
implements(IDistroSeriesSet)
=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py 2010-10-26 15:47:24 +0000
+++ lib/lp/registry/tests/test_distroseries.py 2011-03-10 14:27:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for distroseries."""
@@ -205,6 +205,12 @@
distroseries.getDistroArchSeriesByProcessor(
processorfamily.processors[0]))
+ def test_getDerivedSeries(self):
+ distroseries = self.factory.makeDistroSeries(
+ parent_series=self.factory.makeDistroSeries())
+ self.assertContentEqual(
+ [distroseries], distroseries.parent_series.getDerivedSeries())
+
class TestDistroSeriesPackaging(TestCaseWithFactory):
=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml 2010-12-24 02:22:11 +0000
+++ lib/lp/soyuz/configure.zcml 2011-03-10 14:27:03 +0000
@@ -900,6 +900,16 @@
<allow interface="lp.soyuz.interfaces.distributionjob.IDistributionJob" />
</class>
+ <!-- DistroSeriesDifferenceJobSource -->
+ <class class="lp.soyuz.model.distroseriesdifferencejob.DistroSeriesDifferenceJob">
+ <allow interface="lp.soyuz.interfaces.distributionjob.IDistributionJob" />
+ </class>
+ <securedutility
+ component="lp.soyuz.model.distroseriesdifferencejob.DistroSeriesDifferenceJob"
+ provides="lp.soyuz.interfaces.distroseriesdifferencejob.IDistroSeriesDifferenceJobSource">
+ <allow interface="lp.soyuz.interfaces.distroseriesdifferencejob.IDistroSeriesDifferenceJobSource"/>
+ </securedutility>
+
<!-- SyncPackageJobSource -->
<securedutility
component="lp.soyuz.model.syncpackagejob.SyncPackageJob"
=== modified file 'lib/lp/soyuz/interfaces/distributionjob.py'
--- lib/lp/soyuz/interfaces/distributionjob.py 2010-12-02 16:13:51 +0000
+++ lib/lp/soyuz/interfaces/distributionjob.py 2011-03-10 14:27:03 +0000
@@ -76,6 +76,13 @@
This job copies a single package, optionally including binaries.
""")
+ DISTROSERIESDIFFERENCE = DBItem(3, """
+ Create, delete, or update a Distro Series Difference.
+
+ Updates the status of a potential difference between a derived
+ distribution release series and its parent series.
+ """)
+
class IInitialiseDistroSeriesJobSource(IJobSource):
"""An interface for acquiring IInitialiseDistroSeriesJobs."""
=== added file 'lib/lp/soyuz/interfaces/distroseriesdifferencejob.py'
--- lib/lp/soyuz/interfaces/distroseriesdifferencejob.py 1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/interfaces/distroseriesdifferencejob.py 2011-03-10 14:27:03 +0000
@@ -0,0 +1,24 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""`IDistroSeriesDifferenceJob`."""
+
+__metaclass__ = type
+__all__ = [
+ 'IDistroSeriesDifferenceJobSource',
+ ]
+
+from lp.services.job.interfaces.job import IJobSource
+
+
+class IDistroSeriesDifferenceJobSource(IJobSource):
+ """An `IJob` for creating `DistroSeriesDifference`s."""
+
+ def createForPackagePublication(distroseries, sourcepackagename):
+ """Create jobs as appropriate for a given status publication.
+
+ :param distroseries: A `DistroSeries` that is assumed to be
+ derived from another one.
+ :param sourcepackagename: A `SourcePackageName` that is being
+ published in `distroseries`.
+ """
=== modified file 'lib/lp/soyuz/model/distributionjob.py'
--- lib/lp/soyuz/model/distributionjob.py 2011-01-20 19:39:08 +0000
+++ lib/lp/soyuz/model/distributionjob.py 2011-03-10 14:27:03 +0000
@@ -55,12 +55,16 @@
def __init__(self, distribution, distroseries, job_type, metadata):
super(DistributionJob, self).__init__()
- json_data = simplejson.dumps(metadata)
self.job = Job()
self.distribution = distribution
self.distroseries = distroseries
self.job_type = job_type
- self._json_data = json_data.decode('utf-8')
+ self._json_data = self.serializeMetadata(metadata)
+
+ @classmethod
+ def serializeMetadata(cls, metadata_dict):
+ """Serialize a dict of metadata into a unicode string."""
+ return simplejson.dumps(metadata_dict).decode('utf-8')
@property
def metadata(self):
=== added file 'lib/lp/soyuz/model/distroseriesdifferencejob.py'
--- lib/lp/soyuz/model/distroseriesdifferencejob.py 1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/model/distroseriesdifferencejob.py 2011-03-10 14:27:03 +0000
@@ -0,0 +1,113 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Job class to request generation or update of `DistroSeriesDifference`s."""
+
+__metaclass__ = type
+__all__ = [
+ 'DistroSeriesDifferenceJob',
+ ]
+
+from zope.interface import (
+ classProvides,
+ implements,
+ )
+
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from lp.services.job.model.job import Job
+from lp.soyuz.interfaces.distributionjob import (
+ DistributionJobType,
+ IDistributionJob,
+ )
+from lp.soyuz.model.distributionjob import (
+ DistributionJob,
+ DistributionJobDerived,
+ )
+from lp.soyuz.interfaces.distroseriesdifferencejob import (
+ IDistroSeriesDifferenceJobSource,
+ )
+
+
+def make_metadata(sourcepackagename):
+ """Return JSON metadata for a job on `sourcepackagename`."""
+ return {'sourcepackagename': sourcepackagename.id}
+
+
+def create_job(distroseries, sourcepackagename):
+ """Create a `DistroSeriesDifferenceJob` for a given source package.
+
+ :param distroseries: A `DistroSeries` that is assumed to be derived
+ from another one.
+ :param sourcepackagename: The `SourcePackageName` whose publication
+ history has changed.
+ """
+ job = DistributionJob(
+ distribution=distroseries.distribution, distroseries=distroseries,
+ job_type=DistributionJobType.DISTROSERIESDIFFERENCE,
+ metadata=make_metadata(sourcepackagename))
+ IMasterStore(DistributionJob).add(job)
+ return DistroSeriesDifferenceJob(job)
+
+
+def find_waiting_jobs(distroseries, sourcepackagename):
+ """Look for pending `DistroSeriesDifference` jobs on a package."""
+ # Look for identical pending jobs. This compares directly on
+ # the metadata string. It's fragile, but this is only an
+ # optimization. It's not actually disastrous to create
+ # redundant jobs occasionally.
+ json_metadata = DistributionJob.serializeMetadata(
+ make_metadata(sourcepackagename))
+
+ # Use master store because we don't like outdated information
+ # here.
+ store = IMasterStore(DistributionJob)
+
+ return store.find(
+ DistributionJob,
+ DistributionJob.job_type ==
+ DistributionJobType.DISTROSERIESDIFFERENCE,
+ DistributionJob.distroseries == distroseries,
+ DistributionJob._json_data == json_metadata,
+ DistributionJob.job_id.is_in(Job.ready_jobs))
+
+
+def may_require_job(distroseries, sourcepackagename):
+ """Might publishing this package require a new job?
+
+ Use this to determine whether to create a new
+ `DistroSeriesDifferenceJob`. The answer may possibly be
+ conservatively wrong: the check is really only to save the job
+ runner some unnecessary work, but we don't expect a bit of
+ unnecessary work to be a big problem.
+ """
+ if distroseries is None:
+ return False
+ parent_series = distroseries.parent_series
+ if parent_series is None:
+ return False
+ if parent_series.distribution == distroseries.distribution:
+ # Differences within a distribution are not tracked.
+ return False
+ return find_waiting_jobs(distroseries, sourcepackagename).is_empty()
+
+
+class DistroSeriesDifferenceJob(DistributionJobDerived):
+ """A `Job` type for creating/updating `DistroSeriesDifference`s."""
+
+ implements(IDistributionJob)
+ classProvides(IDistroSeriesDifferenceJobSource)
+
+ class_job_type = DistributionJobType.DISTROSERIESDIFFERENCE
+
+ @classmethod
+ def createForPackagePublication(cls, distroseries, sourcepackagename):
+ """See `IDistroSeriesDifferenceJobSource`."""
+ child_series = distroseries.getDerivedSeries()
+ parent_series = distroseries.parent_series
+ for relative in list(child_series) + [parent_series]:
+ if may_require_job(relative, sourcepackagename):
+ create_job(relative, sourcepackagename)
+
+ def run(self):
+ """See `IRunnableJob`."""
+# TODO: Implement the business end.
=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py 2011-03-08 16:42:43 +0000
+++ lib/lp/soyuz/model/publishing.py 2011-03-10 14:27:03 +0000
@@ -85,6 +85,9 @@
BuildSetStatus,
IBinaryPackageBuildSet,
)
+from lp.soyuz.interfaces.distroseriesdifferencejob import (
+ IDistroSeriesDifferenceJobSource,
+ )
from lp.soyuz.interfaces.publishing import (
active_publishing_status,
IBinaryPackageFilePublishing,
@@ -1422,6 +1425,10 @@
datecreated=UTC_NOW,
ancestor=ancestor)
DistributionSourcePackage.ensure(pub)
+
+ dsd_job_source = getUtility(IDistroSeriesDifferenceJobSource)
+ dsd_job_source.createForPackagePublication(
+ distroseries, sourcepackagerelease.sourcepackagename)
return pub
def getBuildsForSourceIds(
=== added file 'lib/lp/soyuz/tests/test_distroseriesdifferencejob.py'
--- lib/lp/soyuz/tests/test_distroseriesdifferencejob.py 1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_distroseriesdifferencejob.py 2011-03-10 14:27:03 +0000
@@ -0,0 +1,146 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test `DistroSeriesDifferenceJob` and utility."""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+from zope.interface.verify import verifyObject
+
+from canonical.testing.layers import ZopelessDatabaseLayer
+from lp.services.job.interfaces.job import JobStatus
+from lp.soyuz.interfaces.distroseriesdifferencejob import (
+ IDistroSeriesDifferenceJobSource,
+ )
+from lp.soyuz.model.distroseriesdifferencejob import (
+ create_job,
+ find_waiting_jobs,
+ make_metadata,
+ may_require_job,
+ )
+from lp.testing import TestCaseWithFactory
+
+
+class TestDistroSeriesDifferenceJobSource(TestCaseWithFactory):
+ """Tests for `IDistroSeriesDifferenceJobSource`."""
+
+ layer = ZopelessDatabaseLayer
+
+ def getJobSource(self):
+ return getUtility(IDistroSeriesDifferenceJobSource)
+
+ def makeDerivedDistroSeries(self):
+ return self.factory.makeDistroSeries(
+ parent_series=self.factory.makeDistroSeries())
+
+ def test_baseline(self):
+ verifyObject(IDistroSeriesDifferenceJobSource, self.getJobSource())
+
+ def test_make_metadata_is_consistent(self):
+ package = self.factory.makeSourcePackageName()
+ self.assertEqual(make_metadata(package), make_metadata(package))
+
+ def test_make_metadata_distinguishes_packages(self):
+ one_package = self.factory.makeSourcePackageName()
+ another_package = self.factory.makeSourcePackageName()
+ self.assertNotEqual(
+ make_metadata(one_package), make_metadata(another_package))
+
+ def test_may_require_job_accepts_none_distroseries(self):
+ package = self.factory.makeSourcePackageName()
+ self.assertFalse(may_require_job(None, package))
+
+ def test_may_require_job_allows_new_jobs(self):
+ distroseries = self.makeDerivedDistroSeries()
+ package = self.factory.makeSourcePackageName()
+ self.assertTrue(may_require_job(distroseries, package))
+
+ def test_may_require_job_forbids_redundant_jobs(self):
+ distroseries = self.makeDerivedDistroSeries()
+ package = self.factory.makeSourcePackageName()
+ create_job(distroseries, package)
+ self.assertFalse(may_require_job(distroseries, package))
+
+ def test_may_require_job_forbids_jobs_on_nonderived_series(self):
+ sourcepackage = self.factory.makeSourcePackage()
+ self.assertFalse(may_require_job(
+ sourcepackage.distroseries, sourcepackage.sourcepackagename))
+
+ def test_may_require_job_forbids_jobs_for_intra_distro_derivation(self):
+ package = self.factory.makeSourcePackageName()
+ parent = self.factory.makeDistroSeries()
+ child = self.factory.makeDistroSeries(
+ distribution=parent.distribution, parent_series=parent)
+ self.assertFalse(may_require_job(child, package))
+
+ def test_may_require_job_only_considers_waiting_jobs_for_redundancy(self):
+ distroseries = self.makeDerivedDistroSeries()
+ package = self.factory.makeSourcePackageName()
+ existing_job = create_job(distroseries, package)
+ existing_job.job.start()
+ self.assertTrue(may_require_job(distroseries, package))
+
+ def test_create_job_creates_waiting_job(self):
+ distroseries = self.makeDerivedDistroSeries()
+ package = self.factory.makeSourcePackageName()
+ dsdjob = create_job(distroseries, package)
+ self.assertEqual(JobStatus.WAITING, dsdjob.job.status)
+
+ def find_waiting_jobs_finds_waiting_jobs(self):
+ sourcepackage = self.factory.makeSourcePackage()
+ distroseries, sourcepackagename = (
+ sourcepackage.distroseries, sourcepackage.distroseries)
+ job = create_job(distroseries, sourcepackagename)
+ self.assertContentEqual(
+ [job], find_waiting_jobs(distroseries, sourcepackagename))
+
+ def find_waiting_jobs_ignores_other_series(self):
+ sourcepackage = self.factory.makeSourcePackage()
+ distroseries, sourcepackagename = (
+ sourcepackage.distroseries, sourcepackage.distroseries)
+ job = create_job(distroseries, sourcepackagename)
+ other_series = self.factory.makeDistroSeries()
+ self.assertContentEqual(
+ [], find_waiting_jobs(other_series, sourcepackagename))
+
+ def find_waiting_jobs_ignores_other_packages(self):
+ sourcepackage = self.factory.makeSourcePackage()
+ distroseries, sourcepackagename = (
+ sourcepackage.distroseries, sourcepackage.distroseries)
+ job = create_job(distroseries, sourcepackagename)
+ other_spn = self.factory.makeSourcePackageName()
+ self.assertContentEqual(
+ [], find_waiting_jobs(distroseries, other_spn))
+
+ def find_waiting_jobs_considers_only_waiting_jobs(self):
+ sourcepackage = self.factory.makeSourcePackage()
+ distroseries, sourcepackagename = (
+ sourcepackage.distroseries, sourcepackage.distroseries)
+ job = create_job(distroseries, sourcepackagename)
+ job.start()
+ self.assertContentEqual(
+ [], find_waiting_jobs(distroseries, sourcepackagename))
+ job.complete()
+ self.assertContentEqual(
+ [], find_waiting_jobs(distroseries, sourcepackagename))
+
+ def test_createForPackagedPublication_creates_job_for_parent_series(self):
+ derived_series = self.factory.makeDistroSeries(
+ parent_series=self.makeDerivedDistroSeries())
+ package = self.factory.makeSourcePackageName()
+ self.getJobSource().createForPackagePublication(
+ derived_series, package)
+ jobs = list(find_waiting_jobs(derived_series.parent_series, package))
+ self.assertEqual(1, len(jobs))
+ self.assertEqual(package.id, jobs[0].metadata['sourcepackagename'])
+
+ def test_createForPackagePublication_creates_job_for_derived_series(self):
+ derived_series = self.makeDerivedDistroSeries()
+ parent_series = derived_series.parent_series
+ package = self.factory.makeSourcePackageName()
+ self.getJobSource().createForPackagePublication(
+ parent_series, package)
+ jobs = list(find_waiting_jobs(derived_series, package))
+ self.assertEqual(1, len(jobs))
+ self.assertEqual(package.id, jobs[0].metadata['sourcepackagename'])
