[00:04] <StevenK> wgrant: With Component and Section preloaded, down to 70.
[00:09] <wgrant> Excellent
[00:09] <wgrant> What are the rest?
[00:12] <StevenK> wgrant: PackageDiff and related LFA and LFC; SPPH; ArchivePermission
[00:13] <wgrant> Right
[00:18] <StevenK> wgrant: So I think it's reasonable
[00:18] <StevenK> We've dropped 160 queries
[00:52] <StevenK> wgrant: http://pastebin.ubuntu.com/1453695/
[01:15] <StevenK> wgrant: No thoughts at all, I see.
[01:26] <wgrant> StevenK: Hmm
[01:26] <wgrant> Looks reasonable so far
[01:30] <StevenK> wgrant: Hmmm, given IPackageUploadSet.getAll() is the meat inside IDistroSeries.getPackageUploads(), and getAll() does the propertycache dance, I guess the other bug is sorted too?
[01:31] <wgrant> StevenK: It probably is, but you need to check that the delegations to SPR's checker work
[02:08] <wgrant> (by "work" I mean "don't issue tonnes of queries")
[03:31] <StevenK> wgrant: Hmmm, something has broken the upload webservice tests
[03:43] <wgrant> Oh?
[03:44] <StevenK> wgrant: TestPackageUploadWebservice calls factory.makeBuildPackageUpload() (and then creates a BPR, but the factory call does that ...), trying to access upload.builds[0].build leads to an IndexError
[03:45] <wgrant> StevenK: Sounds like something's creating a PUB without using the method that knows to invalidate stuff
[03:46] <StevenK> But it uses addBuild, which does the invalidation!
[03:46] <StevenK>     def addBuild(self, build):
[03:46] <StevenK>         """See `IPackageUpload`."""
[03:46] <StevenK>         del get_property_cache(self).builds
[03:46] <wgrant> It's probably clearing the cache too early
[03:48] <StevenK> We should delay clearing the property cache?
[03:49] <wgrant> Clear the cache once the relevant change has been made
[03:49] <wgrant> Otherwise the cache may be recalculated with the old data
[03:49] <StevenK> Then all three of the methods are wrong
[03:49] <wgrant> Lots of code is wrong
[03:50] <wgrant> Most code is wrong
[03:50] <StevenK> Oh, they used to del get_property_cache, return, the searchable_{name,version}s stuff is now in the middle
[03:55] <StevenK> wgrant: I moved the del to just before the return, but it still dies. Guess I need to create the PUB, delete the property cache and then return
[03:57] <wgrant> StevenK: Right, that's what I meant.
[03:58] <wgrant> It's not necessarily safe to clear the cache before you've made the change
[03:58] <wgrant> Because some operation (eg. accessing a protected attribute) may cause the cache to be recalculated
[03:58] <wgrant> At any time
[03:58] <StevenK>     print upload.builds[0].build
[03:58] <StevenK> IndexError: list index out of range
[03:58] <StevenK> :-(
[04:06] <StevenK> Do not get it. We create the upload in the factory, create a BPB and a BPR, add it to the PU, where we destroy the cached property, and then return it, and the test says upload.builds is []
[04:09] <wgrant> Give me a branch :)
[04:09] <wgrant> Or diff
[04:10] <StevenK> wgrant: http://pastebin.ubuntu.com/1453948/
[04:18] <wgrant> StevenK: Looking
[04:19] <wgrant> Hm
[04:19] <wgrant> StevenK: I wonder if it's not flushing properly
[04:22] <StevenK> wgrant: I was thinking about that -- something that is more aggressive about caching for tests
[04:26] <wgrant> An explicit flush fixes it
[04:26] <wgrant> Odd
[04:26] <StevenK> wgrant: As in a store flush?
[04:27] <wgrant> Yes
[04:28] <wgrant> It looks like the SQLMultipleJoin isn't causing a flush
[04:28] <StevenK> Shall I rewrite them into properties that do a IStore.find() ?
[04:28] <wgrant> No
[04:28] <StevenK> Since SQLMultipleJoin annoys me
[04:28] <wgrant> We should work out why it's not working
[04:30] <wgrant> Oh
[04:30] <wgrant> of course
[04:30] <wgrant> checkPermission is wrapped in @block_implicit_flushes
[04:31] <wgrant> I'd explicitly flush before invalidating the cache in each case
[04:31] <StevenK> Right
[04:33] <StevenK> Ah ha, excellent
[04:34] <StevenK> wgrant: Shall I change that print to a assert just to check that this doesn't bite again?
[04:34] <wgrant> StevenK: No
[04:34] <wgrant> Because it will crash if it fails
[04:34] <wgrant> Already
[04:34] <wgrant> Won't it?
[04:34] <StevenK> No, because I stopped creating a BPR
[04:35] <StevenK> And then the method just iterates over builds, which won't care if it's empty
[04:35] <wgrant> Ah
[04:35] <StevenK> So I can revert the change to create another BPR, which I think is silly, but does test that builds is usable, or an assert
[04:41] <StevenK> Haha, one test depends on the second BPR
[04:41] <StevenK> Eh, back it goes
[04:46] <StevenK> wgrant: So you'd like another test to check query counts?
[04:49] <wgrant> Please
[04:49] <StevenK> wgrant: For IDistroSeries.getPackageUploads() via the Webservice or what?
[04:50] <wgrant> That's what I'd do
[04:51] <wgrant> Given that it's going to be lazr.restful triggering the launchpad.LimitedView checks
[05:04] <StevenK> wgrant: http://pastebin.ubuntu.com/1453995/
[05:09] <wgrant> StevenK: What are those two adjacent SPPH queries?
[05:10] <StevenK> Identical. Odd.
[05:12]  * StevenK tries to work out the backtrace
[05:19] <StevenK> wgrant: Do not get it
[05:20] <StevenK> Also not helped by the backtrace being on one line
[05:36] <StevenK> Gah
[05:36] <StevenK> Can't even convert it to something readable
[08:25] <stub> When does patch-2209-40-3.sql get applied? I merged db-devel when I shouldn't have
[16:44] <mpt> Whoa, I'd forgotten Launchpad used to have "mentorships" ... That's a blast from the past