launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/branchscan-deleted-branch2-602323 into lp:launchpad.

Commit message:
Ensure branch scan job does not oops if a scanned branch is deleted.

Requested reviews:
  Steve Kowalik (stevenk): code
Related bugs:
  Bug #602323 in Launchpad itself: "branch scanner throws LostObjectError if the branch it is scanning is deleted"
  https://bugs.launchpad.net/launchpad/+bug/602323

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/branchscan-deleted-branch2-602323/+merge/133392

== Implementation ==

Ensure branch scan job does not oops if a scanned branch is deleted. Simply catch the lost object exception and log a warning.

Changes in this new version:

- for the celery job infrastructure to see the BranchScanJob created for the test, the test code needs to ensure the current transaction is committed before celery is invoked.
- use standard job metadata to store the branch name instead of a custom attribute

== Tests ==

Write a new TestBranchScanJob test.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/branchjob.py
  lib/lp/code/model/tests/test_branchjob.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/branchscan-deleted-branch2-602323/+merge/133392
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2012-11-07 04:43:07 +0000
+++ lib/lp/code/model/branchjob.py	2012-11-08 03:06:21 +0000
@@ -317,18 +317,35 @@
     @classmethod
     def create(cls, branch):
         """See `IBranchScanJobSource`."""
-        branch_job = BranchJob(branch, cls.class_job_type, {})
+        branch_job = BranchJob(
+            branch, cls.class_job_type, {'branch_name': branch.unique_name})
         return cls(branch_job)
 
+    @property
+    def branch_name(self):
+        return self.metadata['branch_name']
+
     def run(self):
         """See `IBranchScanJob`."""
         from lp.services.scripts import log
         with server(get_ro_server(), no_replace=True):
+<<<<<<< TREE
             lock = try_advisory_lock(
                 LockType.BRANCH_SCAN, self.branch.id, Store.of(self.branch))
             with lock:
                 bzrsync = BzrSync(self.branch, log)
                 bzrsync.syncBranchAndClose()
+=======
+            try:
+                with try_advisory_lock(
+                    LockType.BRANCH_SCAN, self.branch.id,
+                    Store.of(self.branch)):
+                    bzrsync = BzrSync(self.branch, log)
+                    bzrsync.syncBranchAndClose()
+            except LostObjectError:
+                log.warning('Skipping branch %s because it has been deleted.'
+                    % self.branch_name)
+>>>>>>> MERGE-SOURCE
 
     @classmethod
     @contextlib.contextmanager

=== modified file 'lib/lp/services/job/tests/test_celeryjob.py'
--- lib/lp/services/job/tests/test_celeryjob.py	2012-08-08 05:43:18 +0000
+++ lib/lp/services/job/tests/test_celeryjob.py	2012-11-08 03:06:21 +0000
@@ -4,6 +4,7 @@
 from cStringIO import StringIO
 import sys
 from time import sleep
+import transaction
 
 from lazr.jobrunner.bin.clear_queues import clear_queues
 from testtools.content import Content
@@ -39,6 +40,8 @@
 
     def createMissingJob(self):
         job = BranchScanJob.create(self.factory.makeBranch())
+        transaction.commit()
+
         self.addCleanup(drain_celery_queues)
         return job