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

[Paul Hummer]

[Paul Hummer (rockstar)]

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

[Guilherme Salgado (salgado)]

<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)

=== 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 @@
 from lp.code.model.branchmergeproposal import BranchMergeProposal
 from lp.code.model.diff import StaticDiff
 from lp.code.model.revision import RevisionSet
-from lp.codehosting.scanner.bzrsync import BzrSync
+from lp.codehosting.scanner import buglinks, email, mergedetection
+from lp.codehosting.scanner.fixture import (
+    Fixtures, make_zope_event_fixture, run_with_fixture
+)
+from lp.codehosting.scanner.bzrsync import (
+    BzrSync, schedule_diff_updates, schedule_translation_upload
+)
 from lp.codehosting.vfs import (branch_id_to_path, get_multi_server,
     get_scanner_server)
 from lp.services.job.model.job import Job
@@ -264,6 +270,7 @@
 
     classProvides(IBranchScanJobSource)
     class_job_type = BranchJobType.SCAN_BRANCH
+    server = None
 
     @classmethod
     def create(cls, branch):
@@ -273,18 +280,29 @@
 
     def run(self):
         """See `IBranchScanJob`."""
-        bzrsync = BzrSync(self.branch)
+        from canonical.launchpad.scripts import log
+        bzrsync = BzrSync(self.branch, log)
         bzrsync.syncBranchAndClose()
 
-    @staticmethod
+    @classmethod
     @contextlib.contextmanager
-    def contextManager():
+    def contextManager(cls):
         """See `IBranchScanJobSource`."""
         errorlog.globalErrorUtility.configure('branchscanner')
-        server = get_scanner_server()
-        server.setUp()
+        cls.server = get_scanner_server()
+        event_handlers = [
+            email.queue_tip_changed_email_jobs,
+            buglinks.got_new_revision,
+            mergedetection.auto_merge_branches,
+            mergedetection.auto_merge_proposals,
+            schedule_diff_updates,
+            schedule_translation_upload,
+            ]
+        fixture = Fixtures(
+            [cls.server, make_zope_event_fixture(*event_handlers)])
+        fixture.setUp()
         yield
-        server.tearDown()
+        fixture.tearDown()
 
 
 class BranchUpgradeJob(BranchJobDerived):

=== 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 @@
 """Test the scan_branches script."""
 
 
+from storm.locals import Store
 import transaction
 
 from canonical.testing import ZopelessAppServerLayer
 from lp.testing import TestCaseWithFactory
 from canonical.launchpad.scripts.tests import run_script
-from lp.code.model.branchjob import BranchScanJob
+from lp.code.enums import (
+    BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
+    CodeReviewNotificationLevel)
+from lp.code.model.branchjob import BranchJob, BranchJobType, BranchScanJob
+from lp.services.job.model.job import Job, JobStatus
 
 
 class TestScanBranches(TestCaseWithFactory):
@@ -44,11 +49,26 @@
 
         db_branch = self.factory.makeAnyBranch()
         self.make_branch_with_commits_and_scan_job(db_branch)
+        db_branch.subscribe(
+            db_branch.registrant,
+            BranchSubscriptionNotificationLevel.FULL,
+            BranchSubscriptionDiffSize.WHOLEDIFF,
+            CodeReviewNotificationLevel.FULL)
+        transaction.commit()
 
         self.run_script_and_assert_success()
         self.assertEqual(db_branch.revision_count, 3)
 
-    def test_scan_branch_packagebranch(self):
+        store = Store.of(db_branch)
+        result = store.find(
+            BranchJob,
+            BranchJob.jobID == Job.id,
+            Job._status == JobStatus.WAITING,
+            BranchJob.job_type == BranchJobType.REVISION_MAIL,
+            BranchJob.branch == db_branch)
+        self.assertEqual(result.count(), 1)
+
+    def test_scan_packagebranch(self):
         """Test that scan_branches can scan package branches."""
         self.useBzrBranches(real_server=True)
 

=== 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 @@
 
 
 import contextlib
+import logging
 import os
 from signal import getsignal, SIGCHLD, SIGHUP, signal
 import sys
@@ -123,6 +124,8 @@
     def __init__(self, logger=None, error_utility=None):
         self.completed_jobs = []
         self.incomplete_jobs = []
+        if logger is None:
+            logger = logging.getLogger()
         self.logger = logger
         self.error_utility = error_utility
         if self.error_utility is None:
@@ -131,11 +134,16 @@
     def runJob(self, job):
         """Attempt to run a job, updating its status as appropriate."""
         job = IRunnableJob(job)
+
+        self.logger.debug(
+            'Running job in status %s' % (job.status.title,))
+        job.start()
+        transaction.commit()
+
         try:
-            job.start()
-            transaction.commit()
             job.run()
-        except Exception:
+        except Exception, e:
+            self.logger.error(e)
             transaction.abort()
             job.fail()
             # Record the failure.
@@ -209,9 +217,13 @@
         """Run all the Jobs for this JobRunner."""
         for job in self.jobs:
             job = IRunnableJob(job)
+            self.logger.debug(
+                'Trying to acquire lease for job in state %s' % (
+                    job.status.title,))
             try:
                 job.acquireLease()
             except LeaseHeld:
+                self.logger.debug('Could not acquire lease for job')
                 self.incomplete_jobs.append(job)
                 continue
             # Commit transaction to clear the row lock.