[00:08] wgrant, StevenK: do you guys keep track of how many lines of source code you've killed from lp over the last little while? [00:11] I have a graph I update every now and again [00:13] http://people.canonical.com/~cjwatson/tmp/loc-cum.png [00:13] Data per revision in http://people.canonical.com/~cjwatson/tmp/loc [00:14] it started around 500k loc ? [00:14] so down about 15% [00:14] About a million, I think [00:14] ah [00:14] still a nice big number [00:15] Depends on how you count, but by most metrics is 700KLOC-1MLOC [00:31] mwhudson: I've killed close to 40k [00:32] That last big drop on http://people.canonical.com/~cjwatson/tmp/loc-cum.png was me killing YUI 2. [00:46] woo [00:46] killing the source code gently [00:52] StevenK: In https://code.launchpad.net/~stevenk/launchpad/index-previewdiff-merge_proposal/+merge/162915 do you also want to make date_created NOT NULL? [00:53] And you need to rename the column in the ALTER TABLE to branch_merge_proposal [00:53] You should do both NOT NULLs in the same statement to get them done in a single table scan. [00:56] wgrant: Do I need DESC in the index too? [00:56] StevenK: It only makes a very marginal difference in this case, so I wouldn't bother. [00:56] It's important for the bug indices where we have a compound sort in opposing directions. [00:59] StevenK: https://code.launchpad.net/~stevenk/launchpad/switch-bmp-to-previewdiff-merge_proposal/+merge/162916 still pulls out all of the preview diffs just to get the last one [00:59] I'd do [00:59] @cachedproperty [00:59] def preview_diff(self): [00:59] return self._preview_diffs.first() [01:00] wgrant: Ah, but isn't first the earliest diff? [01:00] I can think of an easy solution to that. [01:01] Asc(date_created) ? [01:01] (use .last(), or reverse the sort) [01:01] Ah, so .last() does exist === tasdomas_afk is now known as tasdomas [01:03] wgrant: Index fixed [01:03] StevenK: You also probably need to alter the preloading to populate preview_diff rather than preview_diffs [01:03] Anything that wants multiple BMPs isn't going to care about their historical diffs, just their current ones. [01:04] wgrant: Except that I'm pulling all previewdiffs [01:04] That there is precisely the problem. [01:04] It's unnecessarily pulling an unbounded volume of data. [01:05] It's potentially a large set, particularly before we have pruning implemented. [01:07] wgrant: I agree, but at the time I couldn't think of a way to make certain we only return the latest diff [01:13] StevenK: DISTINCT ON [01:14] SELECT DISTINCT ON (branch_merge_proposal) * FROM previewdiff WHERE branch_merge_proposal IN (1, 2, 3, 4) ORDER BY date_created; [01:14] ish [01:14] That'll return the oldest for each MP, but you get the idea [01:15] I believe I added the .config(distinct=PreviewDiff.branch_merge_proposal) syntax to storm a while ago, so that should be easy. [01:41] wgrant: Both index and switch-bmp have been pushed, and the diff has regenerated. [01:44] StevenK: ITYM Desc [01:44] In the previewdiff preloading [01:44] Asc is the default [01:44] I believe that what you have there will grab the oldest, not the newest [01:44] Which perhaps suggests that your preloading test is incomplete. [01:44] Preloading test? :-) [01:45] There's a preloading test? [01:45] Hopefully :) [01:45] If not then it's not difficult to add. [01:45] wgrant: You tell funny jokes. I'll add one. === tasdomas is now known as tasdomas_afk [03:50] StevenK, lifeless: Do you recall why ~canonical-launchpad exists? [03:51] No [03:51] is it private? [03:51] No [03:51] AFAICT it was only used to own production-auditor, which I've fixed. [03:51] it says ' [03:51] Canonical employees working directly on Launchpad. [03:51] ' [03:51] Which is roughly the definition of ~launchpad [03:51] yeah [03:51] there are specific branch owning teams already [03:52] which the new project policy page references [03:52] Exactly [03:52] And this isn't a member of any teams, and only had privileges in one project [03:52] So I'm not sure why it exists [03:52] Doesn't own any branches, not subscribed to any bugs... === tasdomas_afk is now known as tasdomas [04:56] wgrant: switch-bmp is ready for you again. [05:03] StevenK: Oh, that preloading sort should probably actually be on (bmp, date_created) [05:03] It'll work as it is now, but potentially slowly and it doesn't really make sense [05:24] wgrant: .order_by(PreviewDiff.branch_merge_proposal, Desc(PreviewDiff.date_created) ? [05:24] StevenK: Right [05:37] wgrant: Is that your only remaining concern? [05:37] StevenK: I thiiiiiink so, apart from the pruning thing [05:37] Which we at least need a plan for before this lands [05:38] wgrant: Given your idea that we want only one previewdiff per MP in the short term, I'm happy to put a pipe at the end that adds it [05:41] StevenK: Sounds good [05:41] I'll go over the MPs again for a final check in a sec === tasdomas is now known as tasdomas_afk === tasdomas_afk is now known as tasdomas [06:01] wgrant: I want GROUP BY for this query to only show old previewdiffs? [06:02] StevenK: DISTINCT ON and GROUP BY are usually mutually exclusive [06:02] They perform a similar role, except that GROUP BY gives you aggregates. [06:04] I don't want DISTINCT branch_merge_proposals, though [06:05] StevenK: DISTINCT ON branch_merge_proposal [06:06] Not DISTINCT branch_merge_proposal [06:06] To handle the case that we have 2 old previewdiffs to be pruned. [06:06] Oh [06:06] To show old ones [06:06] Yes [06:06] You want neither [06:06] You'll need window functions or a subquery [06:07] I can see how a window function can help, how does a subquery? [06:07] You can use a subquery to select the latest previewdiff for the MP and then use NOT IN [06:07] Or != [06:16] Bah [06:16] ERROR: window function requires an OVER clause [06:16] And so I add one: [06:17] ERORR: window functions not allowed in WHERE clause [06:18] Sure, you usually have to use a subquery [06:18] So you return the window function result in the select list of a subquery, and filter on that in the outer query [06:24] wgrant: http://paste.ubuntu.com/5646915/ [06:25] StevenK: Right [06:25] It even works [06:49] wgrant: .config(distinct=PreviewDiff.branch_merge_proposal) is not making use of DISTINCT ON [06:51] StevenK: Hm, it possibly needs to be a tuple [06:51] I forget [06:51] I only wrote it [06:52] 39 + if isinstance(select.distinct, (tuple, list)): [06:52] 40 + tokens.append( [06:52] 41 + "ON (%s) " % compile(select.distinct, state, raw=True)) [06:53] Excellent === tasdomas is now known as tasdomas_afk [08:12] StevenK: YUI 2> Yeah, I went and looked at that commit :) [08:13] cjwatson: Haha, as a "What the hell just happened?" sort of check? [08:16] Not really, I just tend to look at big negative commits out of interest === tasdomas_afk is now known as tasdomas === tasdomas is now known as tasdomas_afk === tasdomas_afk is now known as tasdomas === wedgwood_away is now known as wedgwood === tasdomas is now known as tasdomas_afk === wedgwood is now known as wedgwood_away === wedgwood_away is now known as wedgwood === wedgwood is now known as wedgwood_away === tasdomas_afk is now known as tasdomas === wedgwood_away is now known as wedgwood === wedgwood is now known as wedgwood_away === wedgwood_away is now known as wedgwood === wedgwood is now known as wedgwood_away === tasdomas is now known as tasdomas_afk === tasdomas_afk is now known as tasdomas === tasdomas is now known as tasdomas_afk === tasdomas_afk is now known as tasdomas === tasdomas is now known as tasdomas_afk