launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Richard Harding has proposed merging lp:~rharding/launchpad/bugnom_874250 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #874250 in Launchpad itself: "BugNomination:+editstatus timeout for bugs with many tasks"
  https://bugs.launchpad.net/launchpad/+bug/874250

For more details, see:
https://code.launchpad.net/~rharding/launchpad/bugnom_874250/+merge/105317

= 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.
-- 
https://code.launchpad.net/~rharding/launchpad/bugnom_874250/+merge/105317
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/bugnom_874250 into lp:launchpad.

=== 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 14:54:18 +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 14:54:18 +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 14:54:18 +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 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
+
     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 14:54:18 +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 14:54:18 +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 _init_new_task(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._init_new_task(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._init_new_task(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()