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
1=== modified file 'lib/lp/code/model/branchjob.py'
2--- lib/lp/code/model/branchjob.py 2010-01-29 17:15:31 +0000
3+++ lib/lp/code/model/branchjob.py 2010-02-02 01:26:17 +0000
4@@ -48,7 +48,13 @@
5 from lp.code.model.branchmergeproposal import BranchMergeProposal
6 from lp.code.model.diff import StaticDiff
7 from lp.code.model.revision import RevisionSet
8-from lp.codehosting.scanner.bzrsync import BzrSync
9+from lp.codehosting.scanner import buglinks, email, mergedetection
10+from lp.codehosting.scanner.fixture import (
11+ Fixtures, make_zope_event_fixture, run_with_fixture
12+)
13+from lp.codehosting.scanner.bzrsync import (
14+ BzrSync, schedule_diff_updates, schedule_translation_upload
15+)
16 from lp.codehosting.vfs import (branch_id_to_path, get_multi_server,
17 get_scanner_server)
18 from lp.services.job.model.job import Job
19@@ -264,6 +270,7 @@
20
21 classProvides(IBranchScanJobSource)
22 class_job_type = BranchJobType.SCAN_BRANCH
23+ server = None
24
25 @classmethod
26 def create(cls, branch):
27@@ -273,18 +280,29 @@
28
29 def run(self):
30 """See `IBranchScanJob`."""
31- bzrsync = BzrSync(self.branch)
32+ from canonical.launchpad.scripts import log
33+ bzrsync = BzrSync(self.branch, log)
34 bzrsync.syncBranchAndClose()
35
36- @staticmethod
37+ @classmethod
38 @contextlib.contextmanager
39- def contextManager():
40+ def contextManager(cls):
41 """See `IBranchScanJobSource`."""
42 errorlog.globalErrorUtility.configure('branchscanner')
43- server = get_scanner_server()
44- server.setUp()
45+ cls.server = get_scanner_server()
46+ event_handlers = [
47+ email.queue_tip_changed_email_jobs,
48+ buglinks.got_new_revision,
49+ mergedetection.auto_merge_branches,
50+ mergedetection.auto_merge_proposals,
51+ schedule_diff_updates,
52+ schedule_translation_upload,
53+ ]
54+ fixture = Fixtures(
55+ [cls.server, make_zope_event_fixture(*event_handlers)])
56+ fixture.setUp()
57 yield
58- server.tearDown()
59+ fixture.tearDown()
60
61
62 class BranchUpgradeJob(BranchJobDerived):
63
64=== modified file 'lib/lp/code/scripts/tests/test_scan_branches.py'
65--- lib/lp/code/scripts/tests/test_scan_branches.py 2010-01-14 21:46:19 +0000
66+++ lib/lp/code/scripts/tests/test_scan_branches.py 2010-02-02 01:26:17 +0000
67@@ -6,12 +6,17 @@
68 """Test the scan_branches script."""
69
70
71+from storm.locals import Store
72 import transaction
73
74 from canonical.testing import ZopelessAppServerLayer
75 from lp.testing import TestCaseWithFactory
76 from canonical.launchpad.scripts.tests import run_script
77-from lp.code.model.branchjob import BranchScanJob
78+from lp.code.enums import (
79+ BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
80+ CodeReviewNotificationLevel)
81+from lp.code.model.branchjob import BranchJob, BranchJobType, BranchScanJob
82+from lp.services.job.model.job import Job, JobStatus
83
84
85 class TestScanBranches(TestCaseWithFactory):
86@@ -44,11 +49,26 @@
87
88 db_branch = self.factory.makeAnyBranch()
89 self.make_branch_with_commits_and_scan_job(db_branch)
90+ db_branch.subscribe(
91+ db_branch.registrant,
92+ BranchSubscriptionNotificationLevel.FULL,
93+ BranchSubscriptionDiffSize.WHOLEDIFF,
94+ CodeReviewNotificationLevel.FULL)
95+ transaction.commit()
96
97 self.run_script_and_assert_success()
98 self.assertEqual(db_branch.revision_count, 3)
99
100- def test_scan_branch_packagebranch(self):
101+ store = Store.of(db_branch)
102+ result = store.find(
103+ BranchJob,
104+ BranchJob.jobID == Job.id,
105+ Job._status == JobStatus.WAITING,
106+ BranchJob.job_type == BranchJobType.REVISION_MAIL,
107+ BranchJob.branch == db_branch)
108+ self.assertEqual(result.count(), 1)
109+
110+ def test_scan_packagebranch(self):
111 """Test that scan_branches can scan package branches."""
112 self.useBzrBranches(real_server=True)
113
114
115=== modified file 'lib/lp/services/job/runner.py'
116--- lib/lp/services/job/runner.py 2010-01-07 05:03:46 +0000
117+++ lib/lp/services/job/runner.py 2010-02-02 01:26:17 +0000
118@@ -16,6 +16,7 @@
119
120
121 import contextlib
122+import logging
123 import os
124 from signal import getsignal, SIGCHLD, SIGHUP, signal
125 import sys
126@@ -123,6 +124,8 @@
127 def __init__(self, logger=None, error_utility=None):
128 self.completed_jobs = []
129 self.incomplete_jobs = []
130+ if logger is None:
131+ logger = logging.getLogger()
132 self.logger = logger
133 self.error_utility = error_utility
134 if self.error_utility is None:
135@@ -131,11 +134,16 @@
136 def runJob(self, job):
137 """Attempt to run a job, updating its status as appropriate."""
138 job = IRunnableJob(job)
139+
140+ self.logger.debug(
141+ 'Running job in status %s' % (job.status.title,))
142+ job.start()
143+ transaction.commit()
144+
145 try:
146- job.start()
147- transaction.commit()
148 job.run()
149- except Exception:
150+ except Exception, e:
151+ self.logger.error(e)
152 transaction.abort()
153 job.fail()
154 # Record the failure.
155@@ -209,9 +217,13 @@
156 """Run all the Jobs for this JobRunner."""
157 for job in self.jobs:
158 job = IRunnableJob(job)
159+ self.logger.debug(
160+ 'Trying to acquire lease for job in state %s' % (
161+ job.status.title,))
162 try:
163 job.acquireLease()
164 except LeaseHeld:
165+ self.logger.debug('Could not acquire lease for job')
166 self.incomplete_jobs.append(job)
167 continue
168 # Commit transaction to clear the row lock.

Subscribers

People subscribed via source and target branches

to status/vote changes: