Launchpad itself

Merge lp:~abentley/launchpad/twistedjob-enhancements into lp:launchpad

  1. twistedjob-enhancements
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Approved by: Aaron Bentley
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~abentley/launchpad/twistedjob-enhancements
Merge into: lp:launchpad
Diff against target: 254 lines (+117/-6)
8 files modified
lib/lp/code/interfaces/branchmergeproposal.py (+7/-0)
lib/lp/code/model/branchmergeproposaljob.py (+16/-0)
lib/lp/code/model/tests/test_branchmergeproposals.py (+32/-3)
lib/lp/code/scripts/tests/test_update_preview_diffs.py (+28/-3)
lib/lp/services/job/interfaces/job.py (+3/-0)
lib/lp/services/job/model/job.py (+10/-0)
lib/lp/services/job/runner.py (+3/-0)
lib/lp/services/job/tests/test_job.py (+18/-0)
To merge this branch: bzr merge lp:~abentley/launchpad/twistedjob-enhancements
Reviewer Review Type Date Requested Status
Paul Hummer (community) Approve
Review via email: mp+15977@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

= Summary =
Provide misc functionality for converting jobs to use Twisted.

== Proposed fix ==
Add IBranchMergeProposalJob.id and BranchMergeProposalJob.get, which when
invoked in subclasses returns the BranchMergeProposalJob with that id as an
instance of that subclass.

Add an additional message to JobRunner indicating how many jobs did
not complete.

== Pre-implementation notes ==
None of this had a pre-implementation call, though it's basically all required
for the followon branches.

== Implementation details ==
Err...

== Tests ==
bin/test -t test_job -t test_update_preview_diffs -t test_branchmergeproposals

== Demo and Q/A ==
Look at the logs produced by update_preview_diffs on
/x/launchpad.net-logs/scripts/asuka/bzrsyncd. They should have a line that
says "x IUpdatePreviewDiffSource jobs did not complete.".

Revision history for this message
Paul Hummer (rockstar) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
2--- lib/lp/code/interfaces/branchmergeproposal.py 2009-12-07 03:56:18 +0000
3+++ lib/lp/code/interfaces/branchmergeproposal.py 2009-12-18 16:00:38 +0000
4@@ -474,6 +474,10 @@
5 class IBranchMergeProposalJob(Interface):
6 """A Job related to a Branch Merge Proposal."""
7
8+ id = Int(
9+ title=_('DB ID'), required=True, readonly=True,
10+ description=_("The tracking number for this job."))
11+
12 branch_merge_proposal = Object(
13 title=_('The BranchMergeProposal this job is about'),
14 schema=IBranchMergeProposal, required=True)
15@@ -587,6 +591,9 @@
16 def create(bmp):
17 """Create a job to update the diff for this merge proposal."""
18
19+ def get(id):
20+ """Return the UpdatePreviewDiffJob with this id."""
21+
22 def iterReady():
23 """Iterate through jobs ready to update preview diffs."""
24
25
26=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
27--- lib/lp/code/model/branchmergeproposaljob.py 2009-11-03 21:05:14 +0000
28+++ lib/lp/code/model/branchmergeproposaljob.py 2009-12-18 16:00:38 +0000
29@@ -164,6 +164,22 @@
30 return cls(job)
31
32 @classmethod
33+ def get(cls, job_id):
34+ """Get a job by id.
35+
36+ :return: the BranchMergeProposalJob with the specified id, as the
37+ current BranchMergeProposalJobDereived subclass.
38+ :raises: SQLObjectNotFound if there is no job with the specified id,
39+ or its job_type does not match the desired subclass.
40+ """
41+ job = BranchMergeProposalJob.get(job_id)
42+ if job.job_type != cls.class_job_type:
43+ raise SQLObjectNotFound(
44+ 'No object found with id %d and type %s' % (job_id,
45+ cls.class_job_type.title))
46+ return cls(job)
47+
48+ @classmethod
49 def iterReady(klass):
50 """Iterate through all ready BranchMergeProposalJobs."""
51 from lp.code.model.branch import Branch
52
53=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
54--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-08 09:02:43 +0000
55+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-18 16:00:38 +0000
56@@ -41,10 +41,11 @@
57 BRANCH_MERGE_PROPOSAL_FINAL_STATES as FINAL_STATES,
58 IBranchMergeProposal, IBranchMergeProposalGetter, IBranchMergeProposalJob,
59 ICreateMergeProposalJob, ICreateMergeProposalJobSource,
60- IMergeProposalCreatedJob, notify_modified)
61+ IMergeProposalCreatedJob, IUpdatePreviewDiffJobSource, notify_modified)
62 from lp.code.model.branchmergeproposaljob import (
63- BranchMergeProposalJob, BranchMergeProposalJobType,
64- CreateMergeProposalJob, MergeProposalCreatedJob, UpdatePreviewDiffJob)
65+ BranchMergeProposalJob, BranchMergeProposalJobDerived,
66+ BranchMergeProposalJobType, CreateMergeProposalJob,
67+ MergeProposalCreatedJob, UpdatePreviewDiffJob)
68 from lp.code.model.branchmergeproposal import (
69 BranchMergeProposal, BranchMergeProposalGetter, is_valid_transition)
70 from lp.code.model.diff import StaticDiff
71@@ -1235,6 +1236,30 @@
72 verifyObject(IBranchMergeProposalJob, job)
73
74
75+class TestBranchMergeProposalJobDerived(TestCaseWithFactory):
76+ """Test the behaviour of the BranchMergeProposalJobDerived base class."""
77+
78+ layer = LaunchpadZopelessLayer
79+
80+ def test_get(self):
81+ """Ensure get returns or raises appropriately.
82+
83+ It's an error to call get on BranchMergeProposalJobDerived-- it must
84+ be called on a subclass. An object is returned only if the job id
85+ and job type match the request. If no suitable object can be found,
86+ SQLObjectNotFound is raised.
87+ """
88+ bmp = self.factory.makeBranchMergeProposal()
89+ job = MergeProposalCreatedJob.create(bmp)
90+ transaction.commit()
91+ self.assertRaises(
92+ AttributeError, BranchMergeProposalJobDerived.get, job.id)
93+ self.assertRaises(SQLObjectNotFound, UpdatePreviewDiffJob.get, job.id)
94+ self.assertRaises(
95+ SQLObjectNotFound, MergeProposalCreatedJob.get, job.id + 1)
96+ self.assertEqual(job, MergeProposalCreatedJob.get(job.id))
97+
98+
99 class TestMergeProposalCreatedJob(TestCaseWithFactory):
100
101 layer = LaunchpadZopelessLayer
102@@ -1849,6 +1874,10 @@
103
104 layer = LaunchpadZopelessLayer
105
106+ def test_implement_interface(self):
107+ """UpdatePreviewDiffJob implements IUpdatePreviewDiffJobSource."""
108+ verifyObject(IUpdatePreviewDiffJobSource, UpdatePreviewDiffJob)
109+
110 def test_run(self):
111 self.useBzrBranches()
112 bmp = self.createExampleMerge()[0]
113
114=== modified file 'lib/lp/code/scripts/tests/test_update_preview_diffs.py'
115--- lib/lp/code/scripts/tests/test_update_preview_diffs.py 2009-10-17 14:06:03 +0000
116+++ lib/lp/code/scripts/tests/test_update_preview_diffs.py 2009-12-18 16:00:38 +0000
117@@ -11,6 +11,7 @@
118 from canonical.testing import ZopelessAppServerLayer
119 from lp.testing import TestCaseWithFactory
120 from canonical.launchpad.scripts.tests import run_script
121+from canonical.launchpad.webapp import errorlog
122 from lp.code.model.branchmergeproposal import BranchMergeProposal
123 from lp.code.model.branchmergeproposaljob import UpdatePreviewDiffJob
124
125@@ -19,8 +20,7 @@
126
127 layer = ZopelessAppServerLayer
128
129- def test_update_preview_diffs(self):
130- """Ensure update_preview_diffs runs and generates diffs."""
131+ def create_preview_diff_job(self):
132 self.useTempBzrHome()
133 target, target_tree = self.createMirroredBranchAndTree()
134 target_tree.bzrdir.root_transport.put_bytes('foo', 'foo\n')
135@@ -38,11 +38,36 @@
136 job = UpdatePreviewDiffJob.create(bmp)
137 self.assertIs(None, bmp.preview_diff)
138 transaction.commit()
139+ return job, bmp, source_tree
140+
141+ def test_update_preview_diffs(self):
142+ """Ensure update_preview_diffs runs and generates diffs."""
143+ job, bmp, source_tree = self.create_preview_diff_job()
144 retcode, stdout, stderr = run_script(
145 'cronscripts/update_preview_diffs.py', [])
146 self.assertEqual(0, retcode)
147 self.assertEqual('', stdout)
148 self.assertEqual(
149 'INFO creating lockfile\n'
150- 'INFO Ran 1 IUpdatePreviewDiffJobSource jobs.\n', stderr)
151+ 'INFO Ran 1 IUpdatePreviewDiffJobSource jobs.\n'
152+ 'INFO 0 IUpdatePreviewDiffJobSource jobs did not complete.\n',
153+ stderr)
154 self.assertIsNot(None, bmp.preview_diff)
155+
156+ def test_update_preview_diffs_oops(self):
157+ """Ensure update_preview_diffs runs and generates diffs."""
158+ job, bmp, source_tree = self.create_preview_diff_job()
159+ source_tree.bzrdir.root_transport.delete_tree('.bzr')
160+ error_utility = errorlog.ErrorReportingUtility()
161+ error_utility.configure('update_preview_diffs')
162+ old_id = error_utility.getLastOopsReport().id
163+ retcode, stdout, stderr = run_script(
164+ 'cronscripts/update_preview_diffs.py', [])
165+ self.assertEqual(0, retcode)
166+ self.assertIn(
167+ 'INFO 1 IUpdatePreviewDiffJobSource jobs did not complete.\n',
168+ stderr)
169+ self.assertIn('INFO Job resulted in OOPS:', stderr)
170+ new_id = error_utility.getLastOopsReport().id
171+ self.assertNotEqual(old_id, new_id)
172+
173
174=== modified file 'lib/lp/services/job/interfaces/job.py'
175--- lib/lp/services/job/interfaces/job.py 2009-09-02 21:09:48 +0000
176+++ lib/lp/services/job/interfaces/job.py 2009-12-18 16:00:38 +0000
177@@ -82,6 +82,9 @@
178 def acquireLease(duration=300):
179 """Acquire the lease for this Job, or raise LeaseHeld."""
180
181+ def getTimeout():
182+ """Determine how long this job can run before timing out."""
183+
184 def start():
185 """Mark the job as started."""
186
187
188=== modified file 'lib/lp/services/job/model/job.py'
189--- lib/lp/services/job/model/job.py 2009-07-17 00:26:05 +0000
190+++ lib/lp/services/job/model/job.py 2009-12-18 16:00:38 +0000
191@@ -7,6 +7,7 @@
192 __all__ = ['InvalidTransition', 'Job', 'JobStatus']
193
194
195+from calendar import timegm
196 import datetime
197 import time
198
199@@ -84,6 +85,15 @@
200 UTC)
201 self.lease_expires = expiry
202
203+ def getTimeout(self):
204+ """Return the number of seconds until the job should time out.
205+
206+ Jobs timeout when their leases expire. If the lease for this job has
207+ already expired, return 0.
208+ """
209+ expiry = timegm(self.lease_expires.timetuple())
210+ return max(0, expiry - time.time())
211+
212 def start(self):
213 """See `IJob`."""
214 self._set_status(JobStatus.RUNNING)
215
216=== modified file 'lib/lp/services/job/runner.py'
217--- lib/lp/services/job/runner.py 2009-11-26 16:06:55 +0000
218+++ lib/lp/services/job/runner.py 2009-12-18 16:00:38 +0000
219@@ -176,3 +176,6 @@
220 self.logger.info(
221 'Ran %d %s jobs.',
222 len(runner.completed_jobs), self.source_interface.__name__)
223+ self.logger.info(
224+ '%d %s jobs did not complete.',
225+ len(runner.incomplete_jobs), self.source_interface.__name__)
226
227=== modified file 'lib/lp/services/job/tests/test_job.py'
228--- lib/lp/services/job/tests/test_job.py 2009-11-13 06:59:24 +0000
229+++ lib/lp/services/job/tests/test_job.py 2009-12-18 16:00:38 +0000
230@@ -225,6 +225,24 @@
231 job.acquireLease(-1)
232 job.acquireLease()
233
234+ def test_acquireLeaseTimeout(self):
235+ """Test that getTimeout correctly calculates value from lease.
236+
237+ The imprecision is because leases are relative to the current time,
238+ and the current time may have changed by the time we get to
239+ job.getTimeout() <= 300.
240+ """
241+ job = Job()
242+ job.acquireLease(300)
243+ self.assertTrue(job.getTimeout() > 0)
244+ self.assertTrue(job.getTimeout() <= 300)
245+
246+ def test_acquireLeaseTimeoutExpired(self):
247+ """Expired leases don't produce negative timeouts."""
248+ job = Job()
249+ job.acquireLease(-300)
250+ self.assertEqual(0, job.getTimeout())
251+
252
253 def test_suite():
254 return TestLoader().loadTestsFromName(__name__)