[00:58] wgrant: If you remember the duplicated SPPH queries, both are caused deep inside lazr.restful :-( [01:16] StevenK: Why does it make them? [01:17] wgrant: One of them is due to a call to checkAuthenticated, I don't understand the second [01:18] Right [01:18] But lazr.restful doesn't magically make DB queries any more than anything else does [01:18] The fix is usually the same [01:20] wgrant: http://pastebin.ubuntu.com/1486577/ is a diff between the two tracebacks [01:22] StevenK: Right, so it's pretty clear what's going on [01:22] Do you see it? [01:23] wgrant: I wish it was pretty clear to me what's going on. :-( [01:23] Well, look at the traceback [01:23] It's the launchpad.View adapter for SourcePackageRelease [01:23] It calls the security adapter twice to redact fields? [01:23] Possibly delegated from elsewehere [01:25] Oh, one of them is checkAuthenticated and another is checkUnauthenticated? [01:29] No [01:29] One's LimitedView, one's View [01:30] Probably on different objects [01:31] From the traceback we can see that in both cases it's on PackageUpload, which delegates to SourcePackageRelease, which queries the published archives [01:32] wgrant: Oh, in which case there isn't much we can do [01:32] Without stapling cachedproperties into SPR and fiddling with get_property_cache and friends [01:34] But the entire branch is like that [01:34] That's sort of the entire point [01:34] To eliminate exactly this [01:34] Right, but it's one query? :-) [01:35] One query per packageupload [01:35] O(n) queries [01:35] Verboten [01:36] You already presumably precache the PUSs and PUBs [01:36] The SPPHs shouldn't be that awkward [01:46] wgrant: Right, but the SPPH query is coming from the SPR, not the PU [01:47] StevenK: Sure [01:47] Is that a problem? [01:47] You have the SPRs already [01:47] Because you preloaded them [01:47] So you can additionally preload their published archives [01:48] Ahh, but the browser code preloads the SPRs, IPackageUploadSet.getAll() does no such thing. [01:48] Huh? [01:48] If you're not already preloading the SPRs, then you've missed a step [01:49] There must be extra queries before the SPPH queries [01:49] To get the SPRs [01:49] Which means you've preloaded them, in which case you already have them [01:49] Or you haven't preloaded them, in which case you should be preloading them to eliminate O(n) SPR queries [01:51] So, if you're looking at the SPPH queries, you have either already preloaded the SPRs or you need to [01:52] Blink [01:52] 1 PU == 27 queries. 5 PUes == 55 [01:53] Hmm? [01:53] Is that surprising? [01:54] I wasn't expecting that large a jump [01:54] Another ten maybe, not 28 [01:57] That's only <5 queries for each [03:53] StevenK: https://code.launchpad.net/~wgrant/launchpad/bug-1095188/+merge/141579 [03:59] wgrant: Did you have a plan about the test failure? [03:59] Oh [03:59] I thought you might fix that, given you're in the blamelist [03:59] I was just tempted to bump the grace period to 20 years [03:59] That sounds a bit harsh [04:00] wgrant: I decided I wanted to swap back in my knowledge about the PU branch so I could actually finish it off. [04:00] 17 - # I have chosed to use this simple solution for now. [04:00] 18 - # -- kiko, 2006-07-11 [04:01] I'm not sure whether to be scared, horrified or impressed. [04:01] wgrant: return None is implied [04:01] Sure [04:02] But I find it best to be explicit [04:02] But it's +8/-16, so I care little [04:02] wgrant: r=me [04:03] Interesting how it shows r16391 in Unmerged revisions [04:03] The scan failed [04:03] Because of a cold cache [04:03] When I land my testfix in 30s it'll realise r16391 is there [04:03] Because I primed the cache [04:03] Of your branch or devel? [04:03] devel [04:03] I'm a little afraid of your testfix diff, TBH [04:04] It's going to be less terrifying than reworking archiveuploader' [04:04] s datetime usage to be mockable [04:06] That's true [04:09] wgrant: Oh, it's a doctest change. That's okay, then [04:12] Hmm [04:12] I am missing some Launchpad email [04:12] StevenK: Did you get mail about your +1? [04:12] Yeah [04:13] Mysterious [04:13] Timestamp? [04:13] 0303 [04:13] 0403? [04:14] Received by mangled.wedontsleep.org (Postfix, from userid 112) id 5322736BBF; Wed, 2 Jan 2013 15:03:35 +1100 (EST) [04:14] Right, 0403 :) [04:15] I quite mysterously did not get that [04:20] You got the other mail about you proposing it? [04:20] Yep [04:20] Mail queue on indium? [04:32] wgrant: Down from 55 to 34. Still trying to distill out where preloading will still help. [04:33] Everywhere [04:33] Sorry, let me be more clear. [04:33] Still trying to distill out what else should be preloaded to help. [04:34] Well [04:34] Oh, related archives of the SPRs. [04:34] Find things that still make queries [04:34] Preload them :) [05:01] wgrant: Yes, which requires digging through a ton of scrollback because each query gives a seperate traceback [05:01] Anyway, down to 30. [05:01] There are five SPPH queries I can't seem to shake [05:02] If I load_referencing SPPHs and then load_related the Archive, it still makes the queries [05:04] StevenK: eg? [05:05] wgrant: An example query? [05:07] And where it comes from [05:07] And how you are doing the preload [05:08] wgrant: The query is coming from ISourcePackageRelease.published_archives, and I'm doing the preload by: [05:08] publications = load_referencing( [05:08] SourcePackagePublishingHistory, sprs, ['sourcepackagereleaseID']) [05:08] load_related(Archive, publications, ['archiveID']) [05:08] You're missing a critical bit there [05:08] Remember what we discussed two weeks ago? [05:09] wgrant: Setting published_archives to a cachedproperty and using get_property_cache? Or the primary key bit? [05:09] Both [05:09] Because published_archives is a revert PK lookup, Storm can't cache it automatically [05:09] You have to stuff it in manually, with get_property_cache [05:10] s/revert/reverse/ [05:12] Hmmm, so I guess the SPPHs are inconsequtial here. But then I need to map the archives back to the SPR via it [05:13] loltpg [05:13] Right [05:14] Hmm, it may only want the archive id [05:14] This is a slightly abnormal case, because it's a straight double reverse lookup [05:14] The code isn't very clear [05:14] What do you mean? [05:14] return sorted(archives, key=operator.attrgetter('id')) [05:14] published_archives is a collection of Archive objects [05:14] THat returns archives, sorted by id [05:14] Oh, it's the sort key is the id [05:15] Yes [05:15] wgrant: So I can load_related all the archives, but then I need those objects to append using get_property_cache? [05:16] StevenK: You'll probably want to load and keep a list of all the relevant publications, then load all their archives [05:16] Then you can go through each pub, and append pub.archive to the SPR.published_archives cache [05:23] AttributeError: 'DefaultPropertyCache' object has no attribute 'published_archives' [05:23] :-( [05:34] Hm. If I get_property_cache(pubs[0].packageupload) it has sources, builds and customfiles, but my get_property_cache(publication.sourcepackagerelease) has nothing [05:36] StevenK: Do you have it decorated? [05:37] wgrant: Which part? published_archives is decorated by @cachedproperty, yes [05:39] What statement produced that exception? [05:39] spr = get_property_cache(publication.sourcepackagerelease) [05:39] spr.published_archives.append(publication.archive) [05:40] appending to what? [05:40] You can't append to a value that doesn't exist :) [05:41] You need to set it to something first [05:41] spr.published_archives = [] isn't really helpful, since multiple publications might reference the same SPR? [05:42] Sure [05:42] You'll need to set it to [] only if it doesn't already exist [05:43] Or go through and set them all to [] before you start appending anything [05:43] The latter might be better [05:46] Two loops of publications and get_property_cache makes me sad [05:46] Erm why? [05:46] One loop of SPRs to initialise the property caches [05:46] Then one loop of pubs to populate the SPR caches [06:31] wgrant: O HAI, https://code.launchpad.net/~stevenk/launchpad/moar-preload-distroseries-queue/+merge/141581 [06:33] StevenK: So it's 27 queries regardless of PU count? [06:42] wgrant: Yes. Bumping that range(5) to range(6) and running the test still passes. [06:43] wgrant: I'm a little tempted to rip prefill_packageupload_caches out to lp.soyuz.adapters.queue or something [06:50] how long till you've rewritten all of LP ? :) [06:50] lifeless: Shorter if you help. [06:51] StevenK: Heh :). Intruiging suggestion. [06:53] wgrant: My review is still in progress, or I've scared you off? [06:56] #-ops [07:11] StevenK: Why'd you move TestLaunchpadSecurityPolicy_getPrincipalsAccessLevel to ZopelessLayer? [07:12] wgrant: At a guess, that test has no layer at all [07:12] I'm happy to drop that change and the import [07:12] Right, don't add things to layers if they don't need to be there :) [07:13] Layerless tests are much much faster [07:19] wgrant: Still reviewing the packageupload branch, or shall I stop working and move onto rewriting transmission-daemon's postinst in anger? [07:20] I'm reviewing [09:07] good morning [09:34] adeuring: ello were you working on blueprints there for a while before christmas? [09:34] czajkowski: IIRC I did not any serious work on blueprints [09:36] adeuring: ah ok trying to work out what could be happening on a bug [09:36] czajkowski: gah, yes, i did some pivacywork on them [09:36] https://bugs.launchpad.net/launchpad/+bug/1095235 [09:36] <_mup_> Bug #1095235: Bogus dependencies in Blueprint graph < https://launchpad.net/bugs/1095235 > [09:36] ...disclosure... [09:36] but this isn't disclosure but wonring if it may be a knock on effect [09:36] * adeuring needs a memory upgrade [09:36] adeuring: good holiday :) [09:37] czajkowski: ;) anyway, what's the prolem? [09:38] extra dependencies showing up [09:39] czajkowski: is there a bug about anywhere or some other discussion? [09:39] adeuring: the one I just posted in here .. [09:40] czajkowski: thanks (seems need new eyes too) [09:40] :) [09:40] brb I need to reboot [09:42] back [10:05] adeuring: any ideas? [10:06] czajkowski: 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:07] nods [10:07] ok thanks adeuring [10:47] czajkowski: The blueprint dependency code is a little frightening [10:47] StevenK: aloha and welcome back === 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 [23:52] wgrant: Did you peer at the incremental diff? [23:55] StevenK: Looks reasonable [23:58] wgrant: Perhaps we shouldn't hardcode -20 in nascentuploadfile.txt