Merge lp:~rharding/launchpad/bugnom_874250 into lp:launchpad

[Richard Harding]

Support a createManyTasks so that we perform a single query when approving nominations.

= Summary =
This takes another hack at improving the performance of approving the
bug nomination when the bug is listed across many source packages and several
series.

See:
https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-6e6b4b29ef635939095468528e92c7ec

== Pre Implementation ==
Had a chat with Deryck. The goal is to fix the queries doing the 'INSERT INTO
bugtask' by avoiding the loop

== Implementation Notes ==
To do this we add a createManyTasks method and using that when approving bug.

== Q/A ==
We're going to have to try to get a bug nomination such as the bug we're linked against and approve it. It'll still probably oops,but we need to check the oops out and make sure we're only seeing one instance of the 'INSERT INTO bugtask' query that is all over the oops linked in the `Summary`.

== Tests ==

lib/lp/bugs/stories/bugtask-management/xx-change-milestone.txt
lib/lp/bugs/doc/bugtask.txt

== Lint ==
All clean

== LoC Qualification ==
A follow up branch will include porting all of doc/bugtask.txt into unit tests. In order to aid in review, these are done as two branches.

[j.c.sackett (jcsackett)]

This looks good to me. I have some nitpicks below, but nothing blocking.

> === modified file 'lib/lp/bugs/model/bug.py'
> --- lib/lp/bugs/model/bug.py 2012-05-09 06:12:52 +0000
> +++ lib/lp/bugs/model/bug.py 2012-05-10 14:54:18 +0000
> @@ -1237,6 +1237,11 @@
> """See `IBug`."""
> return getUtility(IBugTaskSet).createTask(self, owner, target)
>
> + def addManyTasks(self, owner, targets):
> + """See `IBug`."""
> + new_tasks = getUtility(IBugTaskSet).createManyTasks(self, owner, targets)
> + return new_tasks
> +

Is there any reason to not just have this be?

    def addManyTasks(self, owner, targets):
        """See `IBug`."""
        return getUtility(IBugTaskSet).createManyTasks(self, owner, targets)

> === modified file 'lib/lp/bugs/model/bugtask.py'
> --- lib/lp/bugs/model/bugtask.py 2012-05-09 13:39:12 +0000
> +++ lib/lp/bugs/model/bugtask.py 2012-05-10 14:54:18 +0000
> @@ -1604,11 +1603,8 @@
> params = BugTaskSearchParams(user, **kwargs)
> return self.search(params)
>
> - def createTask(self, bug, owner, target,
> - status=IBugTask['status'].default,
> - importance=IBugTask['importance'].default,
> - assignee=None, milestone=None):
> - """See `IBugTaskSet`."""
> + def _init_new_task(self, bug, owner, target, status, importance, assignee,
> + milestone):
> if not status:
> status = IBugTask['status'].default
> if not importance:

This is a bit nitpicky, but methods, private or otherwise, should still
be camelCase.

review: Approve

[Robert Collins (lifeless)]

> + def addManyTasks(self, owner, targets):
> + """See `IBug`."""
> + new_tasks = getUtility(IBugTaskSet).createManyTasks(self, owner, targets)
> + return new_tasks

That method doesn't seem to have any reason to exist... its
indirection for no benefit. I suggest either using it from IBugTaskSet
or not putting it on IBugTaskSet at all and having the body in IBug.

-Rob

[Richard Harding (rharding)]

Robert, I had considered it, but thought it best to mirror the addTask functionality which has the same indirection. I just replaced the self.bug.addTask with self.bug.addManyTask

If I take out the indirection I can change the call to:

getUtility(IBugtaskSet).addManyTasks(self.bug, approver, targets) I suppose, but now the two add method work differently.

[Robert Collins (lifeless)]

This is a prime source of the fat that makes LP slow to reason about and hard to learn. I don't think consistency here is useful, because its actively increasing the bloat. Feel free to do whatever feels best, I'd be inclined to not have the method on IBug, I think, which will:
 - avoid the indirection
 - be consistent with where the meat of the code is
 - not make changing things later any harder (or easier).

=== modified file 'lib/lp/bugs/doc/bugtask.txt'
--- lib/lp/bugs/doc/bugtask.txt	2012-05-04 05:48:00 +0000
+++ lib/lp/bugs/doc/bugtask.txt	2012-05-10 17:21:19 +0000
@@ -86,6 +86,21 @@
     >>> distro_series_task.distroseries == warty
     True
 
+
+Next we verify that we can create many tasks at a time for multiple targets.
+
+    >>> bug_many = getUtility(IBugSet).get(4)
+    >>> taskset = bugtaskset.createManyTasks(bug_many, mark,
+    ...    [evolution, a_distro, warty], status=BugTaskStatus.FIXRELEASED)
+    >>> tasks = [(t.product, t.distribution, t.distroseries) for t in taskset]
+    >>> tasks.sort()
+    >>> tasks[0][2] == warty
+    True
+    >>> tasks[1][1] == a_distro
+    True
+    >>> tasks[2][0] == evolution
+    True
+
 # XXX: Brad Bollenbach 2005-02-24: See the bottom of this file for a chunk of
 # test documentation that is missing from here, due to problems with resetting
 # the connection after a ProgrammingError is raised. ARGH.

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2012-04-27 05:56:48 +0000
+++ lib/lp/bugs/interfaces/bug.py	2012-05-10 17:21:19 +0000
@@ -779,6 +779,9 @@
     def removeWatch(bug_watch, owner):
         """Remove a bug watch from the bug."""
 
+    def addManyTasks(owners, targets):
+        """Create multiple bug tasks on this bug."""
+
     @call_with(owner=REQUEST_USER)
     @operation_parameters(target=copy_field(IBugTask['target']))
     @export_factory_operation(IBugTask, [])

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2012-04-17 08:04:20 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2012-05-10 17:21:19 +0000
@@ -1570,6 +1570,10 @@
         :return: A list of tuples containing (status_id, count).
         """
 
+    def createManyTasks(bug, owner, targets, status=None, importance=None,
+                   assignee=None, milestone=None):
+        """Create a series of bug tasks and return them."""
+
     def createTask(bug, owner, target, status=None, importance=None,
                    assignee=None, milestone=None):
         """Create a bug task on a bug and return it.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-05-09 06:12:52 +0000
+++ lib/lp/bugs/model/bug.py	2012-05-10 17:21:19 +0000
@@ -1237,6 +1237,10 @@
         """See `IBug`."""
         return getUtility(IBugTaskSet).createTask(self, owner, target)
 
+    def addManyTasks(self, owner, targets):
+        """See `IBug`."""
+        return getUtility(IBugTaskSet).createManyTasks(self, owner, targets)
+
     def addWatch(self, bugtracker, remotebug, owner):
         """See `IBug`."""
         # We shouldn't add duplicate bug watches.

=== modified file 'lib/lp/bugs/model/bugnomination.py'
--- lib/lp/bugs/model/bugnomination.py	2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/model/bugnomination.py	2012-05-10 17:21:19 +0000
@@ -91,8 +91,8 @@
                     targets.append(distroseries)
         else:
             targets.append(self.productseries)
-        for target in targets:
-            bug_task = self.bug.addTask(approver, target)
+        bugtasks = self.bug.addManyTasks(approver, targets)
+        for bug_task in bugtasks:
             self.bug.addChange(BugTaskAdded(UTC_NOW, approver, bug_task))
 
     def decline(self, decliner):

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2012-05-09 13:39:12 +0000
+++ lib/lp/bugs/model/bugtask.py	2012-05-10 17:21:19 +0000
@@ -119,7 +119,10 @@
 from lp.registry.model.pillar import pillar_sort_key
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.services import features
-from lp.services.database.bulk import load_related
+from lp.services.database.bulk import (
+    create,
+    load_related,
+    )
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.enumcol import EnumCol
@@ -150,31 +153,27 @@
           - distro tasks, followed by their distroseries tasks
           - ubuntu first among the distros
     """
+    product_name = None
+    productseries_name = None
+    distribution_name = None
+    distroseries_name = None
+    sourcepackage_name = None
+
     if bugtask.product:
         product_name = bugtask.product.name
-        productseries_name = None
     elif bugtask.productseries:
         productseries_name = bugtask.productseries.name
         product_name = bugtask.productseries.product.name
-    else:
-        product_name = None
-        productseries_name = None
 
     if bugtask.distribution:
         distribution_name = bugtask.distribution.name
-    else:
-        distribution_name = None
 
     if bugtask.distroseries:
         distroseries_name = bugtask.distroseries.version
         distribution_name = bugtask.distroseries.distribution.name
-    else:
-        distroseries_name = None
 
     if bugtask.sourcepackagename:
         sourcepackage_name = bugtask.sourcepackagename.name
-    else:
-        sourcepackage_name = None
 
     # Move ubuntu to the top.
     if distribution_name == 'ubuntu':
@@ -1604,11 +1603,8 @@
         params = BugTaskSearchParams(user, **kwargs)
         return self.search(params)
 
-    def createTask(self, bug, owner, target,
-                   status=IBugTask['status'].default,
-                   importance=IBugTask['importance'].default,
-                   assignee=None, milestone=None):
-        """See `IBugTaskSet`."""
+    def _initNewTask(self, bug, owner, target, status, importance, assignee,
+                       milestone):
         if not status:
             status = IBugTask['status'].default
         if not importance:
@@ -1640,18 +1636,63 @@
             milestone=milestone)
         create_params = non_target_create_params.copy()
         create_params.update(target_key)
+        return create_params, non_target_create_params
+
+    def createManyTasks(self, bug, owner, targets,
+                        status=IBugTask['status'].default,
+                        importance=IBugTask['importance'].default,
+                        assignee=None, milestone=None):
+        """See `IBugTaskSet`."""
+        params = [self._initNewTask(bug, owner, target, status, importance,
+            assignee, milestone) for target in targets]
+
+        fieldnames = (
+            'assignee',
+            'bug',
+            'distribution',
+            'distroseries',
+            'importance',
+            'milestone',
+            'owner',
+            'product',
+            'productseries',
+            'sourcepackagename',
+            '_status',
+        )
+
+        fields = [getattr(BugTask, field) for field in fieldnames]
+        # Values need to be a list of tuples in the order we're declaring our
+        # fields.
+        values = [[p[0].get(field) for field in fieldnames] for p in params]
+
+        taskset = create(fields, values, get_objects=True)
+        del get_property_cache(bug).bugtasks
+        for bugtask in taskset:
+            bugtask.updateTargetNameCache()
+            if bugtask.conjoined_slave:
+                bugtask._syncFromConjoinedSlave()
+        return taskset
+
+    def createTask(self, bug, owner, target,
+                   status=IBugTask['status'].default,
+                   importance=IBugTask['importance'].default,
+                   assignee=None, milestone=None):
+        """See `IBugTaskSet`."""
+        create_params, non_target_create_params = self._initNewTask(bug,
+            owner, target, status, importance, assignee, milestone)
         bugtask = BugTask(**create_params)
-        if target_key['distribution']:
+
+        if create_params['distribution']:
             # Create tasks for accepted nominations if this is a source
             # package addition.
             accepted_nominations = [
                 nomination for nomination in
-                bug.getNominations(target_key['distribution'])
+                bug.getNominations(create_params['distribution'])
                 if nomination.isApproved()]
             for nomination in accepted_nominations:
                 accepted_series_task = BugTask(
                     distroseries=nomination.distroseries,
-                    sourcepackagename=target_key['sourcepackagename'],
+                    sourcepackagename=create_params['sourcepackagename'],
                     **non_target_create_params)
                 accepted_series_task.updateTargetNameCache()