/srv/irclogs.ubuntu.com/2011/02/21/#launchpad-reviews.txt

stublifeless: distro.all_distro_archive_ids issues a db query. Did you consider inlining that?06:21
lifelesswhat do you mean by inlining it ?06:23
lifelessDoing it as part of the query ?06:23
stub'if not len(series_clauses)' seems an odd way of spelling 'if not series_clauses'06:23
stublifeless: Yes. Embedding that logic in the SQL06:24
lifelessstub: I was assuming that other things will hit that cachedproperty06:24
stubIt would depend if that cached_property is being evaluated elsewhere in the pages that use this I guess06:24
lifelessso that inlining it would be of little benefit, and perhaps a (tiny) pessimism06:24
lifelessbut I have no data06:24
lifelesshappy to revisit when we have the next round of scaling data06:25
stublifeless: WHERE (single_item = 'foo') is valid, so you can simplify the OR generation for combined_clause06:28
lifelessok, cool06:28
stublifeless: The helpers we have are what Storm gives us, but I think it would be worse to mix and match Storm & raw SQL here.06:28
lifelessyeah06:29
lifelessI thought about that and cried06:29
stubThis could all be Storm code, but I personally don't believe that it would be more readable or maintainable that way.06:29
stubesp. since you would be jumping to undocumented apis like 'group by'06:30
lifelessagreed06:30
stublifeless: So you have a GROUP BY calculating the MAX(spph.id) for each spr.id, but you don't use it.06:40
lifelessyeah we do06:40
lifelessoh hmmm06:40
lifelesslet me review06:40
stubI don't think that query does what you think that query does06:41
lifelessperhaps06:44
lifelessI think the max is redundant06:46
lifelesswgrant: what do you think, I think you touched the query shape right before me ?06:46
lifelessstub: so the query is transplanted; I'm pretty sure the multi-series change is good; if the query was already faulty, thats perhaps a separate problem ?06:52
stubSELECT DISTINCT ON (spr.id) spr.*06:53
stubFROM06:53
stub    SourcePackageRelease AS spr,06:53
stub    SourcePackagePublishingHistory AS spph,06:53
stub    DistroSeries06:53
stubWHERE06:53
stub    spph.sourcepackagerelease = spr.id06:53
stub    AND spph.distroseries = DistroSeries.id06:53
stub    AND spph.status IN (1,2,3)06:53
stub    AND (06:53
stub        (spr.sourcepackagename IN (1,2,3 )06:53
stub            AND spph.archive IN (1,2,3)06:53
stub            AND DistroSeries.distribution = 106:53
stub        )06:53
stub        )06:53
stubBah06:53
stubForgot the order by06:53
stubSELECT DISTINCT ON (spph.sourcepackagerelease) spr.*06:55
stubFROM06:55
stub    SourcePackageRelease AS spr,06:55
stub    SourcePackagePublishingHistory AS spph,06:55
stub    DistroSeries06:55
stubWHERE06:55
stub    spph.sourcepackagerelease = spr.id06:55
stub    AND spph.distroseries = DistroSeries.id06:55
stub    AND spph.status IN (1,2,3)06:55
stub    AND (06:55
stub        (spr.sourcepackagename IN (1,2,3 )06:55
stub            AND spph.archive IN (1,2,3)06:55
stub            AND DistroSeries.distribution = 106:55
stub        )06:55
stub        )06:55
stubORDER BY spph.sourcepackagerelease DESC, spph.id DESC;06:55
lifelessAIUI we can only get one spph back anyway because of the status check06:56
stubSo it is working by accident?06:56
stubOnly one published with that status in that distroseries... guess not accident.06:56
lifelessstub: I'd be delighted to file a bug to review this query to make it more efficient06:57
lifelessyou'll note the two implementations were different (because one had been tuned more recently) - it was that one that I extended06:57
stubIf you file a bug, you need an XXX: in there stating the query is obviously broken06:59
lifelessI want to consult the soyuz folk first06:59
stubBut I'd rather it got fixed now given we have diagnosed it.06:59
lifelessI'll land a trivial with an XXX if there is an issue06:59
lifelessstub: so you're proposing what you pasted?06:59
stubThe query is broken, we know that.06:59
stubIf you are correct about the status, my query is unnecessarily complicated.07:00
lifelessstub: line 132 in the diff in the mp has the old 2 generations ago query07:00
stubRight. So either the 'latest spph' is unnecessary due to the status field, or someone broke it.07:01
lifelessok07:02
stubThis becomes fairly neat Storm code too...07:06
wgrantlifeless: I don't think I've touched that code recently/ever.07:07
wgrantstub: There will be more than one Published occasionally.07:07
wgrantIn the middle of a publisher run.07:07
stublifeless: So it seems you transplanted the getCurrentSourcePackages query for a distroseries into your new getCurrentSourcePackages for a distribution07:10
lifelessstub: I did07:11
lifelessstub: and adjusted it07:11
lifelesswgrant: so the distroseries code I started with was in fact bong; the max(id) is needed in there as well as the status constraints ?07:12
stubBut that breakage we identified may not matter for a distroseries, but could matter a great deal for a distro.07:12
lifelessstub: distros are already broken07:12
lifelessstub: because they return the most recent for the distro, not the most recent for the distro development focus - so backports can show up07:13
stubSo we are making broken code break faster :)07:13
lifelessin a manner of speaking07:13
lifelessI'd like to delete distro{,Set}.getCurrentSourceReleases07:13
lifelessbut thats a different task07:13
lifelessits not my intent to break the semantics of the functions with this patch07:14
lifelessand I'm glad you noticed07:14
stubBut we are breaking semantics. DistroSet.getCurrentSourceReleases used to return the 'current' source release across all distroseries, including backports (which you say is a bug). The new implementation returns a random sourcepackagerelease with a particular status across all distroseries07:17
stubSo you could get hardy's most recent spr when you asked for Ubuntu's most recent spr.07:18
lifeless1,2 is pending and published07:18
lifelessright, agreed07:19
lifelessso as you say, its broken07:19
stubSuckiest thing is this indicates the tests are not sufficient ;)07:33
lifelesswell07:34
lifelessI hadn't run all the tests07:34
stubreduce(lambda _, series:milestones.extend(series.milestones), self.series_list, [])07:35
stubYou missed your calling as a Perl programmer?07:35
stubOr maybe lisp?07:35
stublifeless: approved with minor comments - https://code.launchpad.net/~lifeless/launchpad/bug-717394/+merge/5054107:56
lifelessstub: huh, I think that goes in the next branch08:05
lifelessstub: sorry about the diff in the -2 branch08:11
lifelessI forgot to give it a prereq08:11
stublifeless: Can you resub easily?08:11
lifelesslet me see08:12
stubOr did the other review get everything and this one is redundant?08:12
lifelessit used to end badly doing htat08:12
lifelesshttps://code.launchpad.net/~lifeless/launchpad/bug-717394-2/+merge/5055708:13
lifelessstub: ^08:16
lifelessstub: diff is there08:20
stuband reviewed08:20
lifelessstub: that leaves just https://code.launchpad.net/~lifeless/launchpad/bug-717394-4/+merge/5054808:24
lifelessstub: did you have a fixed query for bug 636158 ?08:25
_mup_Bug #636158: BugTask:+index times out with many bug tasks/nominations (eg. Bug #1) <lp-bugs> <pg83> <timeout> <Launchpad itself:Triaged> < https://launchpad.net/bugs/636158 >08:25
stublifeless: What query needs fixing in that bug?08:43
lifelessstub: thats the getcurrentsourcereleases one08:44
lifelessthat you were fiddling with08:44
lifelesswhich I broke08:44
lifelessI thought you had a working one I could slot in08:44
stubThe ones I pasted before?08:44
lifelessah08:44
lifelessI was'nt sure that that was complete08:44
stubSELECT DISTINCT ON (spph.sourcepackagerelease) spr.*08:44
stubFROM08:44
stub    SourcePackageRelease AS spr,08:44
stub    SourcePackagePublishingHistory AS spph,08:44
stub    DistroSeries08:44
stubWHERE08:44
stub    spph.sourcepackagerelease = spr.id08:44
stub    AND spph.distroseries = DistroSeries.id08:44
stub    AND spph.status IN (1,2,3)08:44
stub    AND (08:44
lifelessit seemed to have different spph.status ;)08:44
stub        (spr.sourcepackagename IN (1,2,3 )08:44
stub            AND spph.archive IN (1,2,3)08:44
stub            AND DistroSeries.distribution = 108:45
stub        )08:45
stub        )08:45
stubORDER BY spph.sourcepackagerelease DESC, spph.id DESC;08:45
lifelesscool, I'll slot that in08:45
lifelessand similar for DistoSeries I presume ?08:45
stubYes, you need to replace my placeholders :)08:45
lifelessthank you08:45
stubYes, although we might be able to use a simpler query if the status is sufficient for getting 'current package'08:45
stubAlthough from what wgrant said earlier, that might very occasionally return two 'current packages'08:46
lifelessyes08:46
lifelessbelts and bracers for now08:46
stubwgrant: That looks sane to you? ^^08:48
wgrantLooking.08:50
wgrantIf it does return two packages, I wouldn't worry too much.08:50
wgrantThe callsites are just informational.08:50
wgrantJust pick one.08:50
stublifeless: https://code.launchpad.net/~lifeless/launchpad/bug-717394-4/+merge/50548 reviewed08:51
lifelessstub: hi09:37
stubyo09:37
lifelessstub: I don't see how that query actually helps09:37
lifelessspph.sourcepackagerelease isn't going to be distinct if there are two candidates09:38
lifelessand the point is to get sprs for different names at the same time09:38
stubspph.sourcepackagerelease isn't distinct, which is why the DISTINCT ON is there.09:40
lifelesshardy and lucid have different spr's for their most recent spph09:40
lifelesswe want only one of those two sprs09:40
lifelessa window function can do what we need09:41
stubSo we get all (spph, spr) records matching, ordered by (spph.sourcepackagerelease DESC, spph.id DESC) and disgard all but the first matching one09:41
lifelesswhere do we discard ?09:41
stubDISTINCT ON09:41
stubNot DISTINCT09:42
lifelessI get that09:42
lifelessbut I think spph.sourcepackagerelease is different than you think it is09:42
lifelessdistinct on spr.sourcepackagename might do it09:43
lifelesswill the order by take the first item reliably ?09:43
stuboic. we are getting the latest out of all spns....09:43
stubyou get an error if the order by doesn't match what is needed by the ORDER BY09:44
lifelessright, we want multiple sprs, one spr per spn09:44
lifelessI think I have it, trying now09:47
stubSELECT DISTINCT ON (spr.sourcepackagename) spr.*09:47
stubFROM09:47
stub    SourcePackageRelease AS spr,09:47
stub    SourcePackagePublishingHistory AS spph,09:47
stub    DistroSeries09:47
stubWHERE09:47
stub    spph.sourcepackagerelease = spr.id09:47
stub    AND spph.distroseries = DistroSeries.id09:47
stub    AND spph.status IN (1,2,3)09:47
stub    AND (09:47
stub        (spr.sourcepackagename IN (1,2,3 )09:47
stub            AND spph.archive IN (1,2,3)09:47
stub            AND DistroSeries.distribution = 109:47
stub        )09:47
lifelessyes, thats what I have though stormisified09:48
stubYou got DISTINCT ON in Storm syntax?09:48
lifelesstheres a hack09:48
lifelessgrep for DISTINCT ON in lib09:48
lifelessstub: so, thats wrong10:19
lifelessits (spr.sourcepackagename, distroseries.distribution) in the distinct10:19
lifeless*that* gets use unique tuples matching the input parameter10:19
=== henninge_ is now known as henninge
=== mrevell is now known as mrevell-lunch
=== mrevell-lunch is now known as mrevell

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!