launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

The proposal to merge lp:~gary/launchpad/bug723999 into lp:launchpad has been updated.

Description changed to:

This branch makes significant changes to how we calculate the structural subscribers for a given notification.  The previous set-based approach was elegant and may have had similar or even slightly better performance characteristics in some simple cases; however it created gigantic SQL queries and performed poorly when there were a few bugtasks for a bug, and filters for every subscription.

This branch takes a more classic approach to building a SQL query.  In tests, this brought a query that was taking 1.2-1.4 seconds to taking 300 milliseconds or less.

It also significantly reduced the SQL needed.  SQL is 2/3 size in the simplest case with a single bugtask and no tags; 1/2 size in a more complex case with a single bugtask with tags.  Once you have two tasks and on a bug with tags, the new approach's SQL is 1/3 the size, and as you add bugtasks, the ratio gets smaller and smaller.

It's worth noting that Storm support for the WITH statement would really help things.  Then I would do one WITH table for the first query, and in the case of the UNION for the tags, I might do another WITH query for the shared bits.  As it is, I'm trying to compromise between the cost of duplicating work in the database and the cost of making a separate Storm query.  The WITH statement support would also shorten the size of the SQL.  I'll be tempted to try and add that to Storm later, now that Postgres supports it.

I ended up having to use some somewhat esoteric SQL.  In addition to having a pre-implementation call with Danilo about this, I also ran it past him after I was finished.  He seemed pleased with explanations I gave verbally.  I have copiously commented the core function, so I hope that you will be able to follow along there as well.

This is a big branch--noticeably bigger than our 800 line goal. "make lint" forced some of the size on me--it's happy, but at a cost.  However, even before linting, I was still over the limit.  I tried to keep it as small as possible, and have left out some clean-ups that need to happen.  In particular, I tried to make minimal changes to the tests, so some methods that no longer serve any purpose in the codebase are left because they are exercised by valuable tests.  I will follow up with one or more later branches that clean these things up and move tests around appropriately.

Danilo and I agreed that we might want to remove the features of "include_any_tags" (the filter only accepts any bug that has one or more tag) and "exclude_any_tags" (the filter only accepts bugs that have no tags); and we felt that it might be worth reconsidering how the excluded tags work.  However, I kept those out of this branch, as out of scope.

That said, I did make one semantic change to an edge case.  Before, NOT find_all_tags meant (in part) that excluded tags (==NOT include) must *all* match.  I thought that was opposite what I would expect (and in fact initially implemented the other approach without realizing I was contradicting the tests).  After my changes, NOT find_all_tags means that *any* excluded tag must match.  For example, if a bug has the tags "foo" and "bar", a filter with find_all_tags = False and "-foo" (NOT include "foo") will now match because "bar" is not "foo".  (If find_all_tags = True, the filter will not match.  If find_all_tags = False and the filters tags are "-foo" and "-bar", it will not match).  Again, I might like to reconsider the broader approach to this feature in the future, but this is not the branch, or the time for that.

I won't say more here, in the hopes that the comments will guide you.

Thank you for looking at this over-sized branch.

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug723999/+merge/52133
-- 
https://code.launchpad.net/~gary/launchpad/bug723999/+merge/52133
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug723999 into lp:launchpad.