Launchpad itself

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

bugnom_874250
Merge into devel

Proposed by Richard Harding on 2012-05-10

Status:

Merged

Approved by:

Richard Harding on 2012-05-10

Approved revision:

no longer in the source branch.

Merged at revision:

15227

Proposed branch:

lp://staging/~rharding/launchpad/bugnom_874250

Merge into:

lp://staging/launchpad

Diff against target:

218 lines (+88/-21)

6 files modified

lib/lp/bugs/doc/bugtask.txt (+15/-0)
lib/lp/bugs/interfaces/bug.py (+3/-0)
lib/lp/bugs/interfaces/bugtask.py (+4/-0)
lib/lp/bugs/model/bug.py (+4/-0)
lib/lp/bugs/model/bugnomination.py (+2/-2)
lib/lp/bugs/model/bugtask.py (+60/-19)

To merge this branch:

bzr merge lp://staging/~rharding/launchpad/bugnom_874250

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
j.c.sackett (community)		2012-05-10	Approve on 2012-05-10
Review via email: mp+105317@code.staging.launchpad.net

Commit message

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

Description of the change

= 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.

Revision history for this message

j.c.sackett (jcsackett) wrote on 2012-05-10:

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

Revision history for this message

Robert Collins (lifeless) wrote on 2012-05-10:

> + 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

Revision history for this message

Richard Harding (rharding) wrote on 2012-05-10:

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.

Revision history for this message

Robert Collins (lifeless) wrote on 2012-05-10:

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).