Merge lp:~abentley/launchpad/bulk-insert into lp:launchpad

[Aaron Bentley]

Optimize branch scanner.

= Summary =
Partially address bug #808930: Timeout running branch scanner job

== Proposed fix ==
Optimize DB access in the branch scanner by doing bulk row insertions.

This takes a full scan of lp:~irar/gcc-linaro/slp-for-any-nops-4.6/ down to ~2 minutes locally.

== Pre-implementation notes ==
Discussed with gmb

== Implementation details ==
Change INewRevisionEvent to INewMainlineRevisions event, so that multiple revisions can be handled at once. The INewRevisionEvent subscribers only cared about mainline revisions, so providing only mainline revisions simplifies the code path.

Similarly, change got_new_revision to got_new_mainline_revisions.

Change createBranchRevisionFromIDs to perform bulk insertions using new Storm syntax.

Change RevisionSet.acquireRevisionAuthor to acquireRevisionAuthors, so that it does efficient queries for existing RevisionAuthors.

Change RevisionSet.newFromBazaarRevision to newFromBazaarRevisions. It now inserts multiple rows at once, using bulk.create.

Simplify RevisionSet.isPresent (The original did not give a performance advantage.)

syncOneRevision becomes syncRevisions.

Scanner loops use chunks of 10k, as this shows some performance advantage over 1k loops. (100k does not.)

== Tests ==
bin/test -vt test_branchjob -t test_revision -t test_buglinks -t test_bzrsync

== Demo and Q/A ==
None

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/codehosting/scanner/tests/test_bzrsync.py
  lib/lp/code/configure.zcml
  lib/lp/codehosting/scanner/events.py
  lib/lp/code/model/tests/test_branchjob.py
  lib/lp/codehosting/scanner/buglinks.py
  lib/lp/testing/factory.py
  lib/lp/code/interfaces/revision.py
  lib/lp/code/model/branch.py
  lib/lp/codehosting/scanner/tests/test_buglinks.py
  lib/lp/codehosting/scanner/bzrsync.py
  lib/lp/code/model/tests/test_revision.py
  lib/lp/code/model/revision.py
  lib/lp/code/model/branchjob.py

[Aaron Bentley (abentley)]

lifeless has approved LOC increase: "I agree that your branch should land because of decreased costs elsewhere paying for the code..."

[Richard Harding (rharding)]

Overall this looks good. As someone not familiar with the code in question a few comments would have helped. It took me a lot of reads through newFromBazaarRevisions to trace what I think is going on in there. I look forward to a day when we can use namedtuples!

Is there a general frowning on *args? A lot of places needed to be updated to change their calling arguments to be wrapped in a list:
    RevisionSet().newFromBazaarRevisions([bzr_revision])

I can't help but wonder if allowing these to come in as
    def newFromBazaarRevisions(*revisions):

would end up being a bit cleaner. I've not seen much of that practice so far though.

review: Approve (code*)

[Aaron Bentley (abentley)]

> Is there a general frowning on *args?

I don't know if there's a general one, but I tend to avoid them for this kind of use, because it makes it harder to change the signature later. *args and optional arguments don't play well together. When "foo(*branches)" becomes "foo(dry_run=False, *branches)", lots of code breaks, and the method becomes more awkward to call. [And foo(*args, **kwargs) isn't explicit that dry_run is an acceptable kwarg]

I feel like the gain from using *args that way is negligible, and there are real problems, so I avoid it.

I'm comfortable using *args and **kwargs when I'm wrapping things.

Aaron

[j.c.sackett (jcsackett)]

Aaron--

This looks good; I agree with Rick's request for a bit more comments in newFromBazaarRevisions--while it eventually becomes clear what data a given loop or clause is assembling, I wouldn't mind a quick "# Get parent data out of revisions" or similar to help a reader keep track of what's going on where.

Rick--

You're right that the *args format would be cleaner in some cases, but here it looks like the more common case is that the param passed in is a list already, in which case the function would have to do something like `revisions = args[0]`. While I agree it may be a good idea to start raising the use of the *args pattern more, I don't think it's a net gain here.

review: Approve

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2011-12-24 17:49:30 +0000
+++ lib/lp/code/configure.zcml	2012-02-24 02:20:24 +0000
@@ -514,8 +514,8 @@
       for="lp.codehosting.scanner.events.IRevisionsRemoved"
       handler="lp.codehosting.scanner.email.send_removed_revision_emails"/>
   <subscriber
-      for="lp.codehosting.scanner.events.INewRevision"
-      handler="lp.codehosting.scanner.buglinks.got_new_revision"/>
+      for="lp.codehosting.scanner.events.INewMainlineRevisions"
+      handler="lp.codehosting.scanner.buglinks.got_new_mainline_revisions"/>
   <subscriber
       for="lp.codehosting.scanner.events.IScanCompleted"
       handler="lp.codehosting.scanner.mergedetection.auto_merge_proposals"/>

=== modified file 'lib/lp/code/interfaces/revision.py'
--- lib/lp/code/interfaces/revision.py	2011-12-24 16:54:44 +0000
+++ lib/lp/code/interfaces/revision.py	2012-02-24 02:20:24 +0000
@@ -134,8 +134,19 @@
             parent_ids, properties):
         """Create a new Revision with the given revision ID."""
 
-    def newFromBazaarRevision(bzr_revision):
-        """Create a new Revision from the given Bazaar Revision object."""
+    def newFromBazaarRevisions(revision_batch):
+        """Create new Revisions from the given Bazaar Revisions.
+
+        This method allows us to insert Revisions in bulk, which is
+        important for larger branches.
+        """
+
+    def acquireRevisionAuthors(names):
+        """Return a dict of RevisionAuthors with the specified names.
+
+        If a RevisionAuthor is not present in the database, it is created
+        first.
+        """
 
     def getTipRevisionsForBranches(branches):
         """Get the tip branch revisions for the specified branches.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2012-02-20 02:07:55 +0000
+++ lib/lp/code/model/branch.py	2012-02-24 02:20:24 +0000
@@ -28,6 +28,7 @@
     And,
     Count,
     Desc,
+    Insert,
     NamedFunc,
     Not,
     Or,
@@ -132,7 +133,9 @@
     validate_public_person,
     )
 from lp.services.config import config
-from lp.services.database.bulk import load_related
+from lp.services.database.bulk import (
+    load_related,
+    )
 from lp.services.database.constants import (
     DEFAULT,
     UTC_NOW,
@@ -883,13 +886,8 @@
             CREATE TEMPORARY TABLE RevidSequence
             (revision_id text, sequence integer)
             """)
-        data = []
-        for revid, sequence in revision_id_sequence_pairs:
-            data.append('(%s, %s)' % sqlvalues(revid, sequence))
-        data = ', '.join(data)
-        store.execute(
-            "INSERT INTO RevidSequence (revision_id, sequence) VALUES %s"
-            % data)
+        store.execute(Insert(('revision_id', 'sequence'),
+            table=['RevidSequence'], expr=revision_id_sequence_pairs))
         store.execute(
             """
             INSERT INTO BranchRevision (branch, revision, sequence)

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2012-02-15 22:01:27 +0000
+++ lib/lp/code/model/branchjob.py	2012-02-24 02:20:24 +0000
@@ -657,11 +657,10 @@
             merged_revisions = self.getMergedRevisionIDs(revision_id, graph)
             authors = self.getAuthors(merged_revisions, graph)
             revision_set = RevisionSet()
-            rev_authors = set(revision_set.acquireRevisionAuthor(author) for
-                              author in authors)
+            rev_authors = revision_set.acquireRevisionAuthors(authors)
             outf = StringIO()
             pretty_authors = []
-            for rev_author in rev_authors:
+            for rev_author in rev_authors.values():
                 if rev_author.person is None:
                     displayname = rev_author.name
                 else:

=== modified file 'lib/lp/code/model/revision.py'
--- lib/lp/code/model/revision.py	2011-12-30 06:14:56 +0000
+++ lib/lp/code/model/revision.py	2012-02-24 02:20:24 +0000
@@ -25,7 +25,6 @@
     ForeignKey,
     IntCol,
     SQLMultipleJoin,
-    SQLObjectNotFound,
     StringCol,
     )
 from storm.expr import (
@@ -61,6 +60,7 @@
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.projectgroup import IProjectGroup
 from lp.registry.model.person import ValidPersonCache
+from lp.services.database.bulk import create
 from lp.services.database.constants import (
     DEFAULT,
     UTC_NOW,
@@ -73,18 +73,12 @@
 from lp.services.database.sqlbase import (
     quote,
     SQLBase,
-    sqlvalues,
     )
 from lp.services.helpers import shortlist
 from lp.services.identity.interfaces.emailaddress import (
     EmailAddressStatus,
     IEmailAddressSet,
     )
-from lp.services.webapp.interfaces import (
-    DEFAULT_FLAVOR,
-    IStoreSelector,
-    MAIN_STORE,
-    )
 
 
 class Revision(SQLBase):
@@ -277,13 +271,13 @@
             properties = {}
         if _date_created is None:
             _date_created = UTC_NOW
-        author = self.acquireRevisionAuthor(revision_author)
+        authors = self.acquireRevisionAuthors([revision_author])
 
         revision = Revision(
             revision_id=revision_id,
             log_body=log_body,
             revision_date=revision_date,
-            revision_author=author,
+            revision_author=authors[revision_author],
             date_created=_date_created)
         # Don't create future revisions.
         if revision.revision_date > revision.date_created:
@@ -303,22 +297,29 @@
 
         return revision
 
-    def acquireRevisionAuthor(self, name):
-        """Find or create the RevisionAuthor with the specified name.
+    def acquireRevisionAuthors(self, author_names):
+        """Find or create the RevisionAuthors with the specified names.
 
-        Name may be any arbitrary string, but if it is an email-id, and
+        A name may be any arbitrary string, but if it is an email-id, and
         its email address is a verified email address, it will be
         automatically linked to the corresponding Person.
 
         Email-ids come in two major forms:
             "Foo Bar" <[e-mail]>
             [e-mail] (Foo Bar)
+        :return: a dict of name -> RevisionAuthor
         """
-        # create a RevisionAuthor if necessary:
-        try:
-            return RevisionAuthor.byName(name)
-        except SQLObjectNotFound:
-            return self._createRevisionAuthor(name)
+        store = IMasterStore(Revision)
+        author_names = set(author_names)
+        authors = {}
+        for author in store.find(RevisionAuthor,
+                RevisionAuthor.name.is_in(author_names)):
+            authors[author.name] = author
+        missing = author_names - set(authors.keys())
+        # create missing RevisionAuthors
+        for name in missing:
+            authors[name] = self._createRevisionAuthor(name)
+        return authors
 
     def _timestampToDatetime(self, timestamp):
         """Convert the given timestamp to a datetime object.
@@ -339,54 +340,76 @@
         revision_date += timedelta(seconds=timestamp - int_timestamp)
         return revision_date
 
-    def newFromBazaarRevision(self, bzr_revision):
+    def newFromBazaarRevisions(self, revisions):
         """See `IRevisionSet`."""
-        revision_id = bzr_revision.revision_id
-        revision_date = self._timestampToDatetime(bzr_revision.timestamp)
-        authors = bzr_revision.get_apparent_authors()
-        # XXX: JonathanLange 2009-05-01 bug=362686: We can only have one
-        # author per revision, so we use the first on the assumption that
-        # this is the primary author.
-        try:
-            author = authors[0]
-        except IndexError:
-            author = None
-        return self.new(
-            revision_id=revision_id,
-            log_body=bzr_revision.message,
-            revision_date=revision_date,
-            revision_author=author,
-            parent_ids=bzr_revision.parent_ids,
-            properties=bzr_revision.properties)
+
+        # Find all author names for these revisions.
+        author_names = []
+        for bzr_revision in revisions:
+            authors = bzr_revision.get_apparent_authors()
+            try:
+                author = authors[0]
+            except IndexError:
+                author = None
+            author_names.append(author)
+        # Get or make every RevisionAuthor for these revisions.
+        revision_authors = dict(
+            (name, author.id) for name, author in
+            self.acquireRevisionAuthors(author_names).items())
+
+        # Collect all data for making Revision objects.
+        data = []
+        for bzr_revision, author_name in zip(revisions, author_names):
+            revision_id = bzr_revision.revision_id
+            revision_date = self._timestampToDatetime(bzr_revision.timestamp)
+            revision_author = revision_authors[author_name]
+
+            data.append(
+                (revision_id, bzr_revision.message, revision_date,
+                revision_author))
+        # Create all Revision objects.
+        db_revisions = create((
+            Revision.revision_id, Revision.log_body, Revision.revision_date,
+            Revision.revision_author_id), data)
+
+        # Map revision_id to Revision database ID.
+        revision_db_id = dict(
+            (rev.revision_id, rev.id) for rev in db_revisions)
+
+        # Collect all data for making RevisionParent and RevisionProperty
+        # objects.
+        parent_data = []
+        property_data = []
+        for bzr_revision in revisions:
+            db_id = revision_db_id[bzr_revision.revision_id]
+            # Property data: revision DB id, name, value.
+            for name, value in bzr_revision.properties.iteritems():
+                property_data.append((db_id, name, value))
+            parent_ids = bzr_revision.parent_ids
+            # Parent data: revision DB id, sequence, revision_id
+            seen_parents = set()
+            for sequence, parent_id in enumerate(parent_ids):
+                if parent_id in seen_parents:
+                    continue
+                seen_parents.add(parent_id)
+                parent_data.append((db_id, sequence, parent_id))
+        # Create all RevisionParent objects.
+        create((
+            RevisionParent.revisionID, RevisionParent.sequence,
+            RevisionParent.parent_id), parent_data, return_created=False)
+
+        # Create all RevisionProperty objects.
+        create((
+            RevisionProperty.revisionID, RevisionProperty.name,
+            RevisionProperty.value), property_data, return_created=False)
 
     @staticmethod
     def onlyPresent(revids):
         """See `IRevisionSet`."""
-        if not revids:
-            return set()
-        store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-        store.execute(
-            """
-            CREATE TEMPORARY TABLE Revids
-            (revision_id text)
-            """)
-        data = []
-        for revid in revids:
-            data.append('(%s)' % sqlvalues(revid))
-        data = ', '.join(data)
-        store.execute(
-            "INSERT INTO Revids (revision_id) VALUES %s" % data)
-        result = store.execute(
-            """
-            SELECT Revids.revision_id
-            FROM Revids, Revision
-            WHERE Revids.revision_id = Revision.revision_id
-            """)
-        present = set()
-        for row in result.get_all():
-            present.add(row[0])
-        store.execute("DROP TABLE Revids")
-        return present
+        store = IStore(Revision)
+        clause = Revision.revision_id.is_in(revids)
+        present = store.find(Revision.revision_id, clause)
+        return set(present)
 
     def checkNewVerifiedEmail(self, email):
         """See `IRevisionSet`."""

=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2012-02-15 22:01:27 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2012-02-24 02:20:24 +0000
@@ -392,10 +392,9 @@
             existing = branch.getBranchRevision(
                 revision_id=bzr_revision.revision_id)
             if existing is None:
-                revision = RevisionSet().newFromBazaarRevision(bzr_revision)
-            else:
-                revision = RevisionSet().getByRevisionId(
-                    bzr_revision.revision_id)
+                RevisionSet().newFromBazaarRevisions([bzr_revision])
+            revision = RevisionSet().getByRevisionId(
+                bzr_revision.revision_id)
             try:
                 revno = bzr_branch.revision_id_to_revno(revision.revision_id)
             except bzr_errors.NoSuchRevision:

=== modified file 'lib/lp/code/model/tests/test_revision.py'
--- lib/lp/code/model/tests/test_revision.py	2011-12-30 06:14:56 +0000
+++ lib/lp/code/model/tests/test_revision.py	2012-02-24 02:20:24 +0000
@@ -9,6 +9,7 @@
     datetime,
     timedelta,
     )
+from testtools.matchers import Equals
 import time
 from unittest import TestCase
 
@@ -27,7 +28,9 @@
     )
 from lp.registry.model.karma import Karma
 from lp.scripts.garbo import RevisionAuthorEmailLinker
-from lp.services.database.lpstorm import IMasterObject
+from lp.services.database.lpstorm import (
+    IMasterObject,
+    )
 from lp.services.database.sqlbase import cursor
 from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.log.logger import DevNullLogger
@@ -39,11 +42,13 @@
 from lp.testing import (
     login,
     logout,
+    StormStatementRecorder,
     TestCaseWithFactory,
     time_counter,
     )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 
 
 class TestRevisionCreationDate(TestCaseWithFactory):
@@ -257,6 +262,41 @@
         found = self.revision_set.getByRevisionId('nonexistent')
         self.assertIs(None, found)
 
+    def test_newFromBazaarRevisions(self):
+        # newFromBazaarRevisions behaves as expected.
+        # only branchscanner can SELECT revisionproperties.
+        self.becomeDbUser('branchscanner')
+        bzr_revisions = [
+            self.factory.makeBzrRevision('rev-1', prop1="foo"),
+            self.factory.makeBzrRevision('rev-2', parent_ids=['rev-1'])
+        ]
+        with StormStatementRecorder() as recorder:
+            self.revision_set.newFromBazaarRevisions(bzr_revisions)
+        rev_1 = self.revision_set.getByRevisionId('rev-1')
+        self.assertEqual(
+            bzr_revisions[0].committer, rev_1.revision_author.name)
+        self.assertEqual(
+            bzr_revisions[0].message, rev_1.log_body)
+        self.assertEqual(
+            datetime(1970, 1, 1, 0, 0, tzinfo=pytz.UTC), rev_1.revision_date)
+        self.assertEqual([], rev_1.parents)
+        self.assertEqual({'prop1': 'foo'}, rev_1.getProperties())
+        rev_2 = self.revision_set.getByRevisionId('rev-2')
+        self.assertEqual(['rev-1'], rev_2.parent_ids)
+        # Really, less than 9 is great, but if the count improves, we should
+        # tighten this restriction.
+        self.assertThat(recorder, HasQueryCount(Equals(8)))
+
+    def test_acquireRevisionAuthors(self):
+        # AcquireRevisionAuthors creates new authors only if none exists with
+        # that name.
+        author1 = self.revision_set.acquireRevisionAuthors(['name1'])['name1']
+        self.assertEqual(author1.name, 'name1')
+        Store.of(author1).flush()
+        author2 = self.revision_set.acquireRevisionAuthors(['name1'])['name1']
+        self.assertEqual(
+            removeSecurityProxy(author1).id, removeSecurityProxy(author2).id)
+
 
 class TestRevisionGetBranch(TestCaseWithFactory):
     """Test the `getBranch` method of the revision."""

=== modified file 'lib/lp/codehosting/scanner/buglinks.py'
--- lib/lp/codehosting/scanner/buglinks.py	2011-05-27 21:12:25 +0000
+++ lib/lp/codehosting/scanner/buglinks.py	2012-02-24 02:20:24 +0000
@@ -89,8 +89,7 @@
                     registrant=getUtility(ILaunchpadCelebrities).janitor)
 
 
-def got_new_revision(new_revision):
-    if new_revision.isMainline():
-        linker = BugBranchLinker(new_revision.db_branch)
-        linker.createBugBranchLinksForRevision(new_revision.bzr_revision)
-
+def got_new_mainline_revisions(new_mainline_revisions):
+    linker = BugBranchLinker(new_mainline_revisions.db_branch)
+    for bzr_revision in new_mainline_revisions.bzr_revisions:
+        linker.createBugBranchLinksForRevision(bzr_revision)

=== modified file 'lib/lp/codehosting/scanner/bzrsync.py'
--- lib/lp/codehosting/scanner/bzrsync.py	2012-02-15 17:29:54 +0000
+++ lib/lp/codehosting/scanner/bzrsync.py	2012-02-24 02:20:24 +0000
@@ -48,6 +48,7 @@
         if logger is None:
             logger = logging.getLogger(self.__class__.__name__)
         self.logger = logger
+        self.revision_set = getUtility(IRevisionSet)
 
     def syncBranchAndClose(self, bzr_branch=None):
         """Synchronize the database with a Bazaar branch, handling locking.
@@ -97,15 +98,9 @@
         new_db_revs = (
             new_ancestry - getUtility(IRevisionSet).onlyPresent(new_ancestry))
         self.logger.info("Adding %s new revisions.", len(new_db_revs))
-        for revids in iter_list_chunks(list(new_db_revs), 1000):
+        for revids in iter_list_chunks(list(new_db_revs), 10000):
             revisions = self.getBazaarRevisions(bzr_branch, revids)
-            for revision in revisions:
-                # This would probably go much faster if we found some way to
-                # bulk-load multiple revisions at once, but as this is only
-                # executed for revisions new to Launchpad, it doesn't seem
-                # worth it at this stage.
-                self.syncOneRevision(
-                    bzr_branch, revision, revids_to_insert)
+            self.syncRevisions(bzr_branch, revisions, revids_to_insert)
         self.deleteBranchRevisions(branchrevisions_to_delete)
         self.insertBranchRevisions(bzr_branch, revids_to_insert)
         transaction.commit()
@@ -239,24 +234,23 @@
         revisions = bzr_branch.repository.get_parent_map(revisions)
         return bzr_branch.repository.get_revisions(revisions.keys())
 
-    def syncOneRevision(self, bzr_branch, bzr_revision, revids_to_insert):
-        """Import the revision with the given revision_id.
+    def syncRevisions(self, bzr_branch, bzr_revisions, revids_to_insert):
+        """Import the supplied revisions.
 
         :param bzr_branch: The Bazaar branch that's being scanned.
-        :param bzr_revision: the revision to import
+        :param bzr_revisions: the revisions to import
         :type bzr_revision: bzrlib.revision.Revision
         :param revids_to_insert: a dict of revision ids to integer
             revno. Non-mainline revisions will be mapped to None.
         """
-        revision_id = bzr_revision.revision_id
-        revision_set = getUtility(IRevisionSet)
-        # Revision not yet in the database. Load it.
-        self.logger.debug("Inserting revision: %s", revision_id)
-        db_revision = revision_set.newFromBazaarRevision(bzr_revision)
-        notify(
-            events.NewRevision(
-                self.db_branch, bzr_branch, db_revision, bzr_revision,
-                revids_to_insert[revision_id]))
+        self.revision_set.newFromBazaarRevisions(bzr_revisions)
+        mainline_revisions = []
+        for bzr_revision in bzr_revisions:
+            if revids_to_insert[bzr_revision.revision_id] is None:
+                continue
+            mainline_revisions.append(bzr_revision)
+        notify(events.NewMainlineRevisions(
+            self.db_branch, bzr_branch, mainline_revisions))
 
     @staticmethod
     def revisionsToInsert(added_history, last_revno, added_ancestry):
@@ -293,7 +287,7 @@
         self.logger.info("Inserting %d branchrevision records.",
             len(revids_to_insert))
         revid_seq_pairs = revids_to_insert.items()
-        for revid_seq_pair_chunk in iter_list_chunks(revid_seq_pairs, 1000):
+        for revid_seq_pair_chunk in iter_list_chunks(revid_seq_pairs, 10000):
             self.db_branch.createBranchRevisionFromIDs(revid_seq_pair_chunk)
 
     def updateBranchStatus(self, bzr_history):

=== modified file 'lib/lp/codehosting/scanner/events.py'
--- lib/lp/codehosting/scanner/events.py	2010-10-15 18:33:07 +0000
+++ lib/lp/codehosting/scanner/events.py	2012-02-24 02:20:24 +0000
@@ -5,7 +5,7 @@
 
 __metaclass__ = type
 __all__ = [
-    'NewRevision',
+    'NewMainlineRevisions',
     'RevisionsRemoved',
     'TipChanged',
     ]
@@ -32,18 +32,17 @@
         self.bzr_branch = bzr_branch
 
 
-class INewRevision(IObjectEvent):
-    """A new revision has been found in the branch."""
-
-
-class NewRevision(ScannerEvent):
-    """A new revision has been found in the branch."""
-
-    implements(INewRevision)
-
-    def __init__(self, db_branch, bzr_branch, db_revision, bzr_revision,
-                 revno):
-        """Construct a `NewRevision` event.
+class INewMainlineRevisions(IObjectEvent):
+    """A new revision has been found in the branch."""
+
+
+class NewMainlineRevisions(ScannerEvent):
+    """A new revision has been found in the branch."""
+
+    implements(INewMainlineRevisions)
+
+    def __init__(self, db_branch, bzr_branch, bzr_revisions):
+        """Construct a `NewRevisions` event.
 
         :param db_branch: The database branch.
         :param bzr_branch: The Bazaar branch.
@@ -53,13 +52,7 @@
             mainline.
         """
         ScannerEvent.__init__(self, db_branch, bzr_branch)
-        self.db_revision = db_revision
-        self.bzr_revision = bzr_revision
-        self.revno = revno
-
-    def isMainline(self):
-        """Is the new revision a mainline one?"""
-        return self.revno is not None
+        self.bzr_revisions = bzr_revisions
 
 
 class ITipChanged(IObjectEvent):

=== modified file 'lib/lp/codehosting/scanner/tests/test_buglinks.py'
--- lib/lp/codehosting/scanner/tests/test_buglinks.py	2012-02-07 06:48:22 +0000
+++ lib/lp/codehosting/scanner/tests/test_buglinks.py	2012-02-24 02:20:24 +0000
@@ -8,7 +8,6 @@
 from bzrlib.revision import Revision
 from zope.component import getUtility
 from zope.event import notify
-from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
 from lp.bugs.interfaces.bug import IBugSet
@@ -270,10 +269,9 @@
                 revprops={
                     'bugs': 'https://launchpad.net/bugs/%d fixed' % bug.id})
         bzr_revision = tree.branch.repository.get_revision(revision_id)
-        revno = 1
         revision_set = getUtility(IRevisionSet)
-        db_revision = revision_set.newFromBazaarRevision(bzr_revision)
-        notify(events.NewRevision(
-            db_branch, tree.branch, db_revision, bzr_revision, revno))
+        revision_set.newFromBazaarRevisions([bzr_revision])
+        notify(events.NewMainlineRevisions(
+            db_branch, tree.branch, [bzr_revision]))
         bug_branch = getUtility(IBugBranchSet).getBugBranch(bug, db_branch)
         self.assertIsNot(None, bug_branch)

=== modified file 'lib/lp/codehosting/scanner/tests/test_bzrsync.py'
--- lib/lp/codehosting/scanner/tests/test_bzrsync.py	2012-02-21 22:46:28 +0000
+++ lib/lp/codehosting/scanner/tests/test_bzrsync.py	2012-02-24 02:20:24 +0000
@@ -582,8 +582,8 @@
         self.assertIn(merge_id, branchrevisions_to_delete)
 
 
-class TestBzrSyncOneRevision(BzrSyncTestCase):
-    """Tests for `BzrSync.syncOneRevision`."""
+class TestBzrSyncRevisions(BzrSyncTestCase):
+    """Tests for `BzrSync.syncRevisions`."""
 
     def setUp(self):
         BzrSyncTestCase.setUp(self)
@@ -606,7 +606,7 @@
 
         # Sync the revision.  The second parameter is a dict of revision ids
         # to revnos, and will error if the revision id is not in the dict.
-        self.bzrsync.syncOneRevision(None, fake_rev, {'rev42': None})
+        self.bzrsync.syncRevisions(None, [fake_rev], {'rev42': None})
 
         # Find the revision we just synced and check that it has the correct
         # date.

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-02-20 02:07:55 +0000
+++ lib/lp/testing/factory.py	2012-02-24 02:20:24 +0000
@@ -48,6 +48,7 @@
 
 from bzrlib.merge_directive import MergeDirective2
 from bzrlib.plugins.builder.recipe import BaseRecipeBranch
+from bzrlib.revision import Revision as BzrRevision
 import pytz
 from pytz import UTC
 import simplejson
@@ -1545,6 +1546,18 @@
         return merge_proposal.generateIncrementalDiff(
             old_revision, new_revision, diff)
 
+    def makeBzrRevision(self, revision_id=None, parent_ids=None, **kwargs):
+        if revision_id is None:
+            revision_id = self.getUniqueString('revision-id')
+        if parent_ids is None:
+            parent_ids = []
+        return BzrRevision(
+            message=self.getUniqueString('message'),
+            revision_id=revision_id,
+            committer=self.getUniqueString('committer'),
+            parent_ids=parent_ids,
+            timestamp=0, timezone=0, properties=kwargs)
+
     def makeRevision(self, author=None, revision_date=None, parent_ids=None,
                      rev_id=None, log_body=None, date_created=None):
         """Create a single `Revision`."""