StevenKwgrant: If you remember the duplicated SPPH queries, both are caused deep inside lazr.restful :-(00:58
wgrantStevenK: Why does it make them?01:16
StevenKwgrant: One of them is due to a call to checkAuthenticated, I don't understand the second01:17
wgrantBut lazr.restful doesn't magically make DB queries any more than anything else does01:18
wgrantThe fix is usually the same01:18
StevenKwgrant: http://pastebin.ubuntu.com/1486577/ is a diff between the two tracebacks01:20
wgrantStevenK: Right, so it's pretty clear what's going on01:22
wgrantDo you see it?01:22
StevenKwgrant: I wish it was pretty clear to me what's going on. :-(01:23
wgrantWell, look at the traceback01:23
wgrantIt's the launchpad.View adapter for SourcePackageRelease01:23
StevenKIt calls the security adapter twice to redact fields?01:23
wgrantPossibly delegated from elsewehere01:23
StevenKOh, one of them is checkAuthenticated and another is checkUnauthenticated?01:25
wgrantOne's LimitedView, one's View01:29
wgrantProbably on different objects01:30
wgrantFrom the traceback we can see that in both cases it's on PackageUpload, which delegates to SourcePackageRelease, which queries the published archives01:31
StevenKwgrant: Oh, in which case there isn't much we can do01:32
StevenKWithout stapling cachedproperties into SPR and fiddling with get_property_cache and friends01:32
wgrantBut the entire branch is like that01:34
wgrantThat's sort of the entire point01:34
wgrantTo eliminate exactly this01:34
StevenKRight, but it's one query? :-)01:34
wgrantOne query per packageupload01:35
wgrantO(n) queries01:35
wgrantYou already presumably precache the PUSs and PUBs01:36
wgrantThe SPPHs shouldn't be that awkward01:36
StevenKwgrant: Right, but the SPPH query is coming from the SPR, not the PU01:46
wgrantStevenK: Sure01:47
wgrantIs that a problem?01:47
wgrantYou have the SPRs already01:47
wgrantBecause you preloaded them01:47
wgrantSo you can additionally preload their published archives01:47
StevenKAhh, but the browser code preloads the SPRs, IPackageUploadSet.getAll() does no such thing.01:48
wgrantIf you're not already preloading the SPRs, then you've missed a step01:48
wgrantThere must be extra queries before the SPPH queries01:49
wgrantTo get the SPRs01:49
wgrantWhich means you've preloaded them, in which case you already have them01:49
wgrantOr you haven't preloaded them, in which case you should be preloading them to eliminate O(n) SPR queries01:49
wgrantSo, if you're looking at the SPPH queries, you have either already preloaded the SPRs or you need to01:51
StevenK1 PU == 27 queries. 5 PUes == 5501:52
wgrantIs that surprising?01:53
StevenKI wasn't expecting that large a jump01:54
StevenKAnother ten maybe, not 2801:54
wgrantThat's only <5 queries for each01:57
wgrantStevenK: https://code.launchpad.net/~wgrant/launchpad/bug-1095188/+merge/14157903:53
StevenKwgrant: Did you have a plan about the test failure?03:59
wgrantI thought you might fix that, given you're in the blamelist03:59
wgrantI was just tempted to bump the grace period to 20 years03:59
StevenKThat sounds a bit harsh03:59
StevenKwgrant: I decided I wanted to swap back in my knowledge about the PU branch so I could actually finish it off.04:00
StevenK17- # I have chosed to use this simple solution for now.04:00
StevenK18- # -- kiko, 2006-07-1104:00
StevenKI'm not sure whether to be scared, horrified or impressed.04:01
StevenKwgrant: return None is implied04:01
wgrantBut I find it best to be explicit04:02
StevenKBut it's +8/-16, so I care little04:02
StevenKwgrant: r=me04:02
StevenKInteresting how it shows r16391 in Unmerged revisions04:03
wgrantThe scan failed04:03
wgrantBecause of a cold cache04:03
wgrantWhen I land my testfix in 30s it'll realise r16391 is there04:03
wgrantBecause I primed the cache04:03
StevenKOf your branch or devel?04:03
StevenKI'm a little afraid of your testfix diff, TBH04:03
wgrantIt's going to be less terrifying than reworking archiveuploader'04:04
wgrants datetime usage to be mockable04:04
StevenKThat's true04:06
StevenKwgrant: Oh, it's a doctest change. That's okay, then04:09
wgrantI am missing some Launchpad email04:12
wgrantStevenK: Did you get mail about your +1?04:12
StevenKReceived by mangled.wedontsleep.org (Postfix, from userid 112) id 5322736BBF; Wed,  2 Jan 2013 15:03:35 +1100 (EST)04:14
wgrantRight, 0403 :)04:14
wgrantI quite mysterously did not get that04:15
StevenKYou got the other mail about you proposing it?04:20
StevenKMail queue on indium?04:20
StevenKwgrant: Down from 55 to 34. Still trying to distill out where preloading will still help.04:32
StevenKSorry, let me be more clear.04:33
StevenKStill trying to distill out what else should be preloaded to help.04:33
StevenKOh, related archives of the SPRs.04:34
wgrantFind things that still make queries04:34
wgrantPreload them :)04:34
StevenKwgrant: Yes, which requires digging through a ton of scrollback because each query gives a seperate traceback05:01
StevenKAnyway, down to 30.05:01
StevenKThere are five SPPH queries I can't seem to shake05:01
StevenKIf I load_referencing SPPHs and then load_related the Archive, it still makes the queries05:02
wgrantStevenK: eg?05:04
StevenKwgrant: An example query?05:05
wgrantAnd where it comes from05:07
wgrantAnd how you are doing the preload05:07
StevenKwgrant: The query is coming from ISourcePackageRelease.published_archives, and I'm doing the preload by:05:08
StevenKpublications = load_referencing(05:08
StevenK    SourcePackagePublishingHistory, sprs, ['sourcepackagereleaseID'])05:08
StevenKload_related(Archive, publications, ['archiveID'])05:08
wgrantYou're missing a critical bit there05:08
wgrantRemember what we discussed two weeks ago?05:08
StevenKwgrant: Setting published_archives to a cachedproperty and using get_property_cache? Or the primary key bit?05:09
wgrantBecause published_archives is a revert PK lookup, Storm can't cache it automatically05:09
wgrantYou have to stuff it in manually, with get_property_cache05:09
StevenKHmmm, so I guess the SPPHs are inconsequtial here. But then I need to map the archives back to the SPR via it05:12
StevenKHmm, it may only want the archive id05:14
wgrantThis is a slightly abnormal case, because it's a straight double reverse lookup05:14
StevenKThe code isn't very clear05:14
wgrantWhat do you mean?05:14
StevenKreturn sorted(archives, key=operator.attrgetter('id'))05:14
wgrantpublished_archives is a collection of Archive objects05:14
wgrantTHat returns archives, sorted by id05:14
StevenKOh, it's the sort key is the id05:14
StevenKwgrant: So I can load_related all the archives, but then I need those objects to append using get_property_cache?05:15
wgrantStevenK: You'll probably want to load and keep a list of all the relevant publications, then load all their archives05:16
wgrantThen you can go through each pub, and append pub.archive to the SPR.published_archives cache05:16
StevenKAttributeError: 'DefaultPropertyCache' object has no attribute 'published_archives'05:23
StevenKHm. If I get_property_cache(pubs[0].packageupload) it has sources, builds and customfiles, but my get_property_cache(publication.sourcepackagerelease) has nothing05:34
wgrantStevenK: Do you have it decorated?05:36
StevenKwgrant: Which part? published_archives is decorated by @cachedproperty, yes05:37
wgrantWhat statement produced that exception?05:39
StevenK        spr = get_property_cache(publication.sourcepackagerelease)05:39
StevenK        spr.published_archives.append(publication.archive)05:39
wgrantappending to what?05:40
wgrantYou can't append to a value that doesn't exist :)05:40
wgrantYou need to set it to something first05:41
StevenKspr.published_archives = [] isn't really helpful, since multiple publications might reference the same SPR?05:41
wgrantYou'll need to set it to [] only if it doesn't already exist05:42
wgrantOr go through and set them all to [] before you start appending anything05:43
wgrantThe latter might be better05:43
StevenKTwo loops of publications and get_property_cache makes me sad05:46
wgrantErm why?05:46
wgrantOne loop of SPRs to initialise the property caches05:46
wgrantThen one loop of pubs to populate the SPR caches05:46
StevenKwgrant: O HAI, https://code.launchpad.net/~stevenk/launchpad/moar-preload-distroseries-queue/+merge/14158106:31
wgrantStevenK: So it's 27 queries regardless of PU count?06:33
StevenKwgrant: Yes. Bumping that range(5) to range(6) and running the test still passes.06:42
StevenKwgrant: I'm a little tempted to rip prefill_packageupload_caches out to lp.soyuz.adapters.queue or something06:43
lifelesshow long till you've rewritten all of LP ? :)06:50
StevenKlifeless: Shorter if you help.06:50
lifelessStevenK: Heh :). Intruiging suggestion.06:51
StevenKwgrant: My review is still in progress, or I've scared you off?06:53
wgrantStevenK: Why'd you move TestLaunchpadSecurityPolicy_getPrincipalsAccessLevel to ZopelessLayer?07:11
StevenKwgrant: At a guess, that test has no layer at all07:12
StevenKI'm happy to drop that change and the import07:12
wgrantRight, don't add things to layers if they don't need to be there :)07:12
wgrantLayerless tests are much much faster07:13
StevenKwgrant: Still reviewing the packageupload branch, or shall I stop working and move onto rewriting transmission-daemon's postinst in anger?07:19
wgrantI'm reviewing07:20
adeuringgood morning09:07
czajkowskiadeuring: ello were you working on blueprints there for a while before christmas?09:34
adeuringczajkowski: IIRC I did not any serious work on blueprints09:34
czajkowskiadeuring: ah ok trying to work out what could be happening on a bug09:36
adeuringczajkowski: gah, yes, i did some pivacywork on them09:36
_mup_Bug #1095235: Bogus dependencies in Blueprint graph <Launchpad itself:New> < https://launchpad.net/bugs/1095235 >09:36
czajkowskibut this isn't disclosure but wonring if it may be a knock on effect09:36
* adeuring needs a memory upgrade09:36
czajkowskiadeuring: good holiday :)09:36
adeuringczajkowski: ;) anyway, what's the prolem?09:37
czajkowskiextra dependencies showing up09:38
adeuringczajkowski: is there a bug about anywhere or some other discussion?09:39
czajkowskiadeuring: the one I just posted in here ..09:39
adeuringczajkowski: thanks (seems need new eyes too)09:40
czajkowskibrb I need to reboot09:40
czajkowskiadeuring: any ideas?10:05
adeuringczajkowski: not really for now. I think that's something for the maintenance team. At first, I'd do an SQL query to see what dependencies really exist in the DB and go on from there.10:06
czajkowskiok thanks adeuring10:07
StevenKczajkowski: The blueprint dependency code is a little frightening10:47
czajkowskiStevenK: aloha and welcome back10:47
=== yofel_ is now known as yofel
=== slank_away is now known as slank
=== teknico_ is now known as teknico
=== abentley changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On-call reviewer: abentley | Firefighting: - | Critical bugs: <150
=== deryck is now known as deryck[lunch]
=== almaisan-away is now known as al-maisan
=== al-maisan is now known as almaisan-away
=== deryck[lunch] is now known as deryck
=== BradCrittenden is now known as bac
=== abentley changed the topic of #launchpad-dev to: http://dev.launchpad.net/ | On-call reviewer: - | Firefighting: - | Critical bugs: <150
StevenKwgrant: Did you peer at the incremental diff?23:52
wgrantStevenK: Looks reasonable23:55
StevenKwgrant: Perhaps we shouldn't hardcode -20 in nascentuploadfile.txt23:58

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