Launchpad itself

Merge lp:~rockstar/launchpad/scanner-events into lp:launchpad/db-devel

  1. scanner-events
  2. Merge into db-devel
Proposed by Paul Hummer
Status: Merged
Merged at revision: not available
Proposed branch: lp:~rockstar/launchpad/scanner-events
Merge into: lp:launchpad/db-devel
Diff against target: 168 lines (+62/-12)
3 files modified
lib/lp/code/model/branchjob.py (+25/-7)
lib/lp/code/scripts/tests/test_scan_branches.py (+22/-2)
lib/lp/services/job/runner.py (+15/-3)
To merge this branch: bzr merge lp:~rockstar/launchpad/scanner-events
Reviewer Review Type Date Requested Status
Guilherme Salgado (community) code Approve
Review via email: mp+18281@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Paul Hummer (rockstar) wrote :

Hi there-

  The branch scanner was re-written this last cycle to use the job system.
However, the script was not setting up the Zope event infrastructure, and so
the scanner was firing events but no one was listening. This meant that
revision mail jobs were not getting created, so users weren't getting their
revision mail.

  Aaron wrote the original fix code and tested on staging, but the LOSAs
wouldn't less us deploy the cowboy, so I took over, wrote the tests and
am shepherding the branch now.

Cheers,
Paul

Revision history for this message
Guilherme Salgado (salgado) wrote :

<salgado> rockstar, ISTM that .run() now expects self.server to be not-None, but do we have anything to make sure contextManager() is called before run(), to make sure self.server is set?
<rockstar> salgado, it's actually cls.server, but yes, the JobCronScript deals with all of that.
<rockstar> salgado, it even uses the context manager stuff that barry posted to the list about.
<salgado> rockstar, would it be worth having an "assert self.server is not None" in run(), just to make it clear?
<rockstar> salgado, well, run is called in a with statement, so it would bomb out before then.
<salgado> also, what do you think of a test showing that all the expected subscribers are actually registered for the event? the existing one only shows (implicitly) that the subscriber for generating diffs is registered, but not the others
<rockstar> AIUI, the with is what handles the contextManager call, and knows how to clean up after itself.
<rockstar> salgado, well, I don't think this fix is the way forward. abentley, thumper, and I are all in agreement there. It's the how we're not quite sure of.
<abentley> salgado: This isn't a nice fix, this is a quick, minimally-invasive fix.
<salgado> ok, maybe that'll be clear for someone reading the whole thing and not just the diff, but maybe a comment explaining how we know self.server will be set before run() is called would be nice
<rockstar> salgado, maybe, but this isn't the only place that uses the contextManager.
<salgado> fair enough, I leave that up to you
<rockstar> If you're implementing JobCronScript, you need to know about contextManager and how it works.
<salgado> but if you're just reading the code, it might take you longer than necessary to understand it

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py 2010-01-29 17:15:31 +0000
+++ lib/lp/code/model/branchjob.py 2010-02-02 01:26:17 +0000
@@ -48,7 +48,13 @@
48from lp.code.model.branchmergeproposal import BranchMergeProposal48from lp.code.model.branchmergeproposal import BranchMergeProposal
49from lp.code.model.diff import StaticDiff49from lp.code.model.diff import StaticDiff
50from lp.code.model.revision import RevisionSet50from lp.code.model.revision import RevisionSet
51from lp.codehosting.scanner.bzrsync import BzrSync51from lp.codehosting.scanner import buglinks, email, mergedetection
52from lp.codehosting.scanner.fixture import (
53 Fixtures, make_zope_event_fixture, run_with_fixture
54)
55from lp.codehosting.scanner.bzrsync import (
56 BzrSync, schedule_diff_updates, schedule_translation_upload
57)
52from lp.codehosting.vfs import (branch_id_to_path, get_multi_server,58from lp.codehosting.vfs import (branch_id_to_path, get_multi_server,
53 get_scanner_server)59 get_scanner_server)
54from lp.services.job.model.job import Job60from lp.services.job.model.job import Job
@@ -264,6 +270,7 @@
264270
265 classProvides(IBranchScanJobSource)271 classProvides(IBranchScanJobSource)
266 class_job_type = BranchJobType.SCAN_BRANCH272 class_job_type = BranchJobType.SCAN_BRANCH
273 server = None
267274
268 @classmethod275 @classmethod
269 def create(cls, branch):276 def create(cls, branch):
@@ -273,18 +280,29 @@
273280
274 def run(self):281 def run(self):
275 """See `IBranchScanJob`."""282 """See `IBranchScanJob`."""
276 bzrsync = BzrSync(self.branch)283 from canonical.launchpad.scripts import log
284 bzrsync = BzrSync(self.branch, log)
277 bzrsync.syncBranchAndClose()285 bzrsync.syncBranchAndClose()
278286
279 @staticmethod287 @classmethod
280 @contextlib.contextmanager288 @contextlib.contextmanager
281 def contextManager():289 def contextManager(cls):
282 """See `IBranchScanJobSource`."""290 """See `IBranchScanJobSource`."""
283 errorlog.globalErrorUtility.configure('branchscanner')291 errorlog.globalErrorUtility.configure('branchscanner')
284 server = get_scanner_server()292 cls.server = get_scanner_server()
285 server.setUp()293 event_handlers = [
294 email.queue_tip_changed_email_jobs,
295 buglinks.got_new_revision,
296 mergedetection.auto_merge_branches,
297 mergedetection.auto_merge_proposals,
298 schedule_diff_updates,
299 schedule_translation_upload,
300 ]
301 fixture = Fixtures(
302 [cls.server, make_zope_event_fixture(*event_handlers)])
303 fixture.setUp()
286 yield304 yield
287 server.tearDown()305 fixture.tearDown()
288306
289307
290class BranchUpgradeJob(BranchJobDerived):308class BranchUpgradeJob(BranchJobDerived):
291309
=== modified file 'lib/lp/code/scripts/tests/test_scan_branches.py'
--- lib/lp/code/scripts/tests/test_scan_branches.py 2010-01-14 21:46:19 +0000
+++ lib/lp/code/scripts/tests/test_scan_branches.py 2010-02-02 01:26:17 +0000
@@ -6,12 +6,17 @@
6"""Test the scan_branches script."""6"""Test the scan_branches script."""
77
88
9from storm.locals import Store
9import transaction10import transaction
1011
11from canonical.testing import ZopelessAppServerLayer12from canonical.testing import ZopelessAppServerLayer
12from lp.testing import TestCaseWithFactory13from lp.testing import TestCaseWithFactory
13from canonical.launchpad.scripts.tests import run_script14from canonical.launchpad.scripts.tests import run_script
14from lp.code.model.branchjob import BranchScanJob15from lp.code.enums import (
16 BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
17 CodeReviewNotificationLevel)
18from lp.code.model.branchjob import BranchJob, BranchJobType, BranchScanJob
19from lp.services.job.model.job import Job, JobStatus
1520
1621
17class TestScanBranches(TestCaseWithFactory):22class TestScanBranches(TestCaseWithFactory):
@@ -44,11 +49,26 @@
4449
45 db_branch = self.factory.makeAnyBranch()50 db_branch = self.factory.makeAnyBranch()
46 self.make_branch_with_commits_and_scan_job(db_branch)51 self.make_branch_with_commits_and_scan_job(db_branch)
52 db_branch.subscribe(
53 db_branch.registrant,
54 BranchSubscriptionNotificationLevel.FULL,
55 BranchSubscriptionDiffSize.WHOLEDIFF,
56 CodeReviewNotificationLevel.FULL)
57 transaction.commit()
4758
48 self.run_script_and_assert_success()59 self.run_script_and_assert_success()
49 self.assertEqual(db_branch.revision_count, 3)60 self.assertEqual(db_branch.revision_count, 3)
5061
51 def test_scan_branch_packagebranch(self):62 store = Store.of(db_branch)
63 result = store.find(
64 BranchJob,
65 BranchJob.jobID == Job.id,
66 Job._status == JobStatus.WAITING,
67 BranchJob.job_type == BranchJobType.REVISION_MAIL,
68 BranchJob.branch == db_branch)
69 self.assertEqual(result.count(), 1)
70
71 def test_scan_packagebranch(self):
52 """Test that scan_branches can scan package branches."""72 """Test that scan_branches can scan package branches."""
53 self.useBzrBranches(real_server=True)73 self.useBzrBranches(real_server=True)
5474
5575
=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py 2010-01-07 05:03:46 +0000
+++ lib/lp/services/job/runner.py 2010-02-02 01:26:17 +0000
@@ -16,6 +16,7 @@
1616
1717
18import contextlib18import contextlib
19import logging
19import os20import os
20from signal import getsignal, SIGCHLD, SIGHUP, signal21from signal import getsignal, SIGCHLD, SIGHUP, signal
21import sys22import sys
@@ -123,6 +124,8 @@
123 def __init__(self, logger=None, error_utility=None):124 def __init__(self, logger=None, error_utility=None):
124 self.completed_jobs = []125 self.completed_jobs = []
125 self.incomplete_jobs = []126 self.incomplete_jobs = []
127 if logger is None:
128 logger = logging.getLogger()
126 self.logger = logger129 self.logger = logger
127 self.error_utility = error_utility130 self.error_utility = error_utility
128 if self.error_utility is None:131 if self.error_utility is None:
@@ -131,11 +134,16 @@
131 def runJob(self, job):134 def runJob(self, job):
132 """Attempt to run a job, updating its status as appropriate."""135 """Attempt to run a job, updating its status as appropriate."""
133 job = IRunnableJob(job)136 job = IRunnableJob(job)
137
138 self.logger.debug(
139 'Running job in status %s' % (job.status.title,))
140 job.start()
141 transaction.commit()
142
134 try:143 try:
135 job.start()
136 transaction.commit()
137 job.run()144 job.run()
138 except Exception:145 except Exception, e:
146 self.logger.error(e)
139 transaction.abort()147 transaction.abort()
140 job.fail()148 job.fail()
141 # Record the failure.149 # Record the failure.
@@ -209,9 +217,13 @@
209 """Run all the Jobs for this JobRunner."""217 """Run all the Jobs for this JobRunner."""
210 for job in self.jobs:218 for job in self.jobs:
211 job = IRunnableJob(job)219 job = IRunnableJob(job)
220 self.logger.debug(
221 'Trying to acquire lease for job in state %s' % (
222 job.status.title,))
212 try:223 try:
213 job.acquireLease()224 job.acquireLease()
214 except LeaseHeld:225 except LeaseHeld:
226 self.logger.debug('Could not acquire lease for job')
215 self.incomplete_jobs.append(job)227 self.incomplete_jobs.append(job)
216 continue228 continue
217 # Commit transaction to clear the row lock.229 # Commit transaction to clear the row lock.

Subscribers

People subscribed via source and target branches

to status/vote changes: