Merge lp:~mwhudson/launchpad/puller-doesnt-call-startMirroring into lp:launchpad

[Michael Hudson-Doyle]

Remove a now-redundant call to startMirroring in the puller worker.

[Michael Hudson-Doyle (mwhudson)]

Hi,

This branch removes a superfluous call to startMirroring from the puller system -- it's called by acquireBranchToPull already.

More changes to tests than code...

Cheers,
mwh

[Tim Penhey (thumper)]

Looks good.

I also noticed your comment on IRC about how you thought it may be possible to have two workers pulling the same branch. In normal circumstances it won't be, however I just thought of an edge case where it is currently pulling a branch, and a new requestMirror comes in, will another worker try to pull it again?

review: Approve

[Michael Hudson-Doyle (mwhudson)]

> Looks good.
>
> I also noticed your comment on IRC about how you thought it may be possible to
> have two workers pulling the same branch. In normal circumstances it won't
> be, however I just thought of an edge case where it is currently pulling a
> branch, and a new requestMirror comes in, will another worker try to pull it
> again?

Yeah, that's exactly what I mean.

Cheers,
mwh

=== modified file 'lib/lp/codehosting/puller/scheduler.py'
--- lib/lp/codehosting/puller/scheduler.py	2009-09-28 17:54:00 +0000
+++ lib/lp/codehosting/puller/scheduler.py	2009-10-01 22:50:24 +0000
@@ -366,10 +366,8 @@
 
     def startMirroring(self):
         self.logger.info(
-            'Mirroring branch %d: %s to %s', self.branch_id, self.source_url,
-            self.destination_url)
-        return self.branch_puller_endpoint.callRemote(
-            'startMirroring', self.branch_id)
+            'Worker started on branch %d: %s to %s', self.branch_id,
+            self.source_url, self.destination_url)
 
     def mirrorFailed(self, reason, oops):
         self.logger.info('Recorded %s', oops)

=== modified file 'lib/lp/codehosting/puller/tests/test_scheduler.py'
--- lib/lp/codehosting/puller/tests/test_scheduler.py	2009-07-29 06:02:12 +0000
+++ lib/lp/codehosting/puller/tests/test_scheduler.py	2009-10-01 22:50:24 +0000
@@ -445,12 +445,11 @@
                 self.eventHandler.unique_name), oops.url)
 
     def test_startMirroring(self):
-        deferred = self.eventHandler.startMirroring()
+        # startMirroring does not send a message to the endpoint.
+        deferred = defer.maybeDeferred(self.eventHandler.startMirroring)
 
         def checkMirrorStarted(ignored):
-            self.assertEqual(
-                [('startMirroring', self.arbitrary_branch_id)],
-                self.status_client.calls)
+            self.assertEqual([], self.status_client.calls)
 
         return deferred.addCallback(checkMirrorStarted)
 
@@ -477,7 +476,7 @@
 
     def test_mirrorComplete(self):
         arbitrary_revision_id = 'rev1'
-        deferred = self.eventHandler.startMirroring()
+        deferred = defer.maybeDeferred(self.eventHandler.startMirroring)
 
         def mirrorSucceeded(ignored):
             self.status_client.calls = []
@@ -494,7 +493,7 @@
     def test_mirrorFailed(self):
         arbitrary_error_message = 'failed'
 
-        deferred = self.eventHandler.startMirroring()
+        deferred = defer.maybeDeferred(self.eventHandler.startMirroring)
 
         def mirrorFailed(ignored):
             self.status_client.calls = []
@@ -704,8 +703,7 @@
 
         def check_authserver_called(ignored):
             self.assertEqual(
-                [('startMirroring', self.db_branch.id),
-                 ('setStackedOn', 77, ''),
+                [('setStackedOn', 77, ''),
                  ('mirrorComplete', self.db_branch.id, revision_id)],
                 self.client.calls)
             return ignored
@@ -963,19 +961,8 @@
                 script_text=lower_timeout_script)
             deferred = puller_master.mirror()
             def check_mirror_failed(ignored):
-                # In Bazaar 1.15, set_stacked_on_url locks the branch. With
-                # 1.14, there's an extra 'setStackedOn' call to the XML-RPC
-                # server, which is wrong. With 1.15, that call no longer takes
-                # place.
-                #
-                # Assert that the call length is 2 (not 3) when we upgrade to
-                # Bazaar 1.15.
-                self.assertIn(len(self.client.calls), [2, 3])
-                start_mirroring_call = self.client.calls[0]
-                mirror_failed_call = self.client.calls[-1]
-                self.assertEqual(
-                    start_mirroring_call,
-                    ('startMirroring', self.db_branch.id))
+                self.assertEqual(len(self.client.calls), 1)
+                mirror_failed_call = self.client.calls[0]
                 self.assertEqual(
                     mirror_failed_call[:2],
                     ('mirrorFailed', self.db_branch.id))