[00:50] wgrant: Hmmm, how do I return a 400. [00:58] StevenK: Maybe raise a BadRequest, but I'm not sure if you can at that point. [00:59] I just tried with HTTPBadRequest and that OOPSes [01:06] wgrant: BadRequest also OOPSes [01:07] I might be using the wrong one, since I'm pulling it from lazr.restfulclinet [01:07] *restfulclient [01:08] StevenK: HTTPBadRequest is probably the right one [01:08] But the request may not be sufficiently set up by then [01:08] What does the traceback look like? [01:09] wgrant: http://pastebin.ubuntu.com/1595337/ [01:10] StevenK: Ah, so the publisher deals with it really early [01:10] Might best to go the decode('utf-8', 'replace') route [01:10] Right, like PATH_INFO ? [01:11] wgrant: My test might be screwing that up [01:12] But given it's three lines, maybe not [01:12] Like PATH_INFO, yes [01:13] If they don't speak UTF-8 then they're not worth understanding :) [01:19] wgrant: http://pastebin.ubuntu.com/1595363/ [01:19] StevenK: Heh, no [01:20] wgrant: Would you prefer _decode() turns into return unicode(text, 'utf-8 [01:20] Bleh [01:20] StevenK: Well, think about what your diff will do [01:20] return unicode(text, 'utf-8', 'replace') even [01:20] It'll go through all the user-specified charsets [01:20] Decoding each with replacements [01:21] Then decode as UTF-8 with replacements [01:21] What you might want to do is try to decode as the user-specified charsets [01:21] Then, if they all fail, decode as UTF-8 replacements [01:21] *or* just go ahead and decode as UTF-8 with replace to start with [01:21] I'd prefer the former [01:23] wgrant: http://pastebin.ubuntu.com/1595366/ [01:23] StevenK: No [01:24] Oh, missing a break [01:25] It's also wrong :) [01:25] That'll decode as utf-8 with replace if any decoding fails [01:25] You want to do it if *all* decodings fail [01:25] So perhaps you want to [01:25] def _decode(self, text): [01:25] text = super(LaunchpadBrowserRequest, self)._decode(text) [01:26] if isinstance(text, str): [01:26] text = test.decode('utf-8', 'replace') [01:26] return text [01:26] s/test/text/ [01:26] Heh [01:28] wgrant: That sounds good. Even with an explanatory comment. [01:29] BrowserRequest does not love adapting LBR [01:29] So I'll call it directly [01:29] ? [01:29] Oh, is it not new-style? [01:30] envadapter = IUserPreferredCharsets(self) [01:30] TypeError: ('Could not adapt', , ) [01:30] Ah [01:30] That's odd... [01:30] We don't implment IBrowserRequest [01:30] That might be it === slank is now known as slank_away [01:31] Except that we inherit that method and it works fine [01:32] When I copied and pasted it, it was failing with the IUserPreferredCharsets line, but I just removed it rather than looking for the import [01:32] Sure, but _decode is called today and seems to work fine, right? [01:34] LBR doesn't have a _decode today [01:34] It will fall to the subclasses [01:34] Exactly [01:34] Which have the same self [01:34] But work fine [01:34] Ah, hm [01:34] implementing IBrowserRequest helps not [01:36] Neither does calling text = BrowserRequest._decode(self, text) [01:38] StevenK: So you're getting that crash in a browser? [01:38] That's from my test [01:38] Let me try the browser [01:39] Oh [01:39] You might not even be in the right layer [01:40] TestBasicLaunchpadRequest has no layer [01:40] That would be a problem for a test that desires to make use of the Component Architecture. [01:40] TestLaunchpadBrowserRequest doesn't either [01:41] wgrant: It works in the browser, so yeah, layers [01:42] None of the testcases in services/webapp/tests/test_servers use layers at all [01:43] Right, none of them need to at present [01:43] LFLayer, or is that too high? [01:44] FunctionalLayer should be fine [01:44] Don't even need DatabaseFunctionalLayer [01:46] wgrant: http://pastebin.ubuntu.com/1595398/ [01:47] StevenK: Might want to extend the comment to state that we always want unicode, dammit. [01:49] wgrant: http://pastebin.ubuntu.com/1595412/ [01:51] StevenK: Seems reasonable. [01:51] Checked that it fixes the crash? [01:51] It renders the page with the replaced string in the widget [01:51] So 'good enough' [01:52] Yep [01:58] StevenK: I'll have ~3600 lines of diff for you soon :) [01:58] Over 6 branches, though [01:58] Whimper [02:05] :) [02:17] StevenK: So, do you understand what I'm doing? [02:36] https://code.launchpad.net/~wgrant/launchpad/flatten-bfj-1-populate/+merge/145541 https://code.launchpad.net/~wgrant/launchpad/flatten-bfj-2-garbo/+merge/145542 https://code.launchpad.net/~wgrant/launchpad/flatten-bfj-3-query/+merge/145543 https://code.launchpad.net/~wgrant/launchpad/flatten-bfj-3.5-more-query/+merge/145819 https://code.launchpad.net/~wgrant/launchpad/flatten-bfj-4-app-eliminate/+merge/145820 [02:37] StevenK: They're ready for you to review at your leisure [02:55] wgrant: You never explained your dastardly plan [03:04] StevenK: I've outlined it in the MP descs [03:04] But can go into more detail if you desire [03:05] wgrant: Yeah [03:05] To which? :) [03:06] The first comment [03:06] Ah, good :) [03:06] It's conceptually simple, there's just a lot of code to change [03:06] wgrant: I've approved -1-populate with one niggle, but it took me a few minutes to figure out the the _new_* gubbins [03:06] Hopefully they lose their _new_ness [03:06] Yes [03:07] They're just there so they don't override the properties which delegate to PB/BFJ [03:07] So all the other callsites don't have to care [03:07] (since the _new_ cols obviously won't be populated when the appservers first start up with that code) [03:07] They'll be renamed in -5, I suspect [03:07] Yeah [03:08] -5-scorched-earth ? :-) [03:29] * StevenK hits line of 525 of -3.5 and vomits [03:30] Mmm? [03:30] 520 + BinaryPackageBuild, Archive).find( [03:30] 521 + BinaryPackageBuild, [03:30] 522 SQL(query)) [03:30] 523 resultset.order_by( [03:31] It's sort of disgusting [03:31] And needs a rewrite [03:31] Ah yeah, a bit, but not in this branch :) [03:31] It needs Stormification [03:31] Which should come as part of the search rewrite in -6 [03:31] Heh [03:32] Which branch does DROP TABLE PackageBuild, then? [03:32] -7 or so, because it's not quite so urgent :) [03:33] Maybe -8 can make retry-depwait a garbo-frequently job [03:33] * StevenK hides [03:33] No [03:33] It takes too long [03:34] It's already a tunable loop, and could maybe be in -hourly [03:34] Even with this change it will be too slow? [03:34] Yes [03:34] This won't affect it in any significant fashion [03:36] wgrant: Up to -4 [03:39] StevenK: Right, I originally considered having a common garbo base, but the column lists and joins for each are somewhat different, and the code will be on prod for just hours [03:39] Right, exactly why I ended up with 'eh' [03:40] wgrant: I'm done, all approved. -3.5 has the most niggles [03:40] Thanks [03:54] StevenK: Can you reapprove https://code.launchpad.net/~wgrant/launchpad/flatten-bfj-3-query/+merge/146035 (no changes other than a merge from a garbo-dropping branch, which required a resubmit) [03:55] Your first comment will be resolved in a later branch that rewrites the search stuff, and your second is invalid (see the next couple of lines) [03:55] 1013+ find_spec = find_spec + (order_by_table,) [03:55] OH [03:56] That's hideous [03:56] Quite! [03:56] wgrant: Re-approved [03:56] Thanks [04:04] wgrant: Blink. My test failed on buildbot, but passes locally. [04:04] 2.7 versus 2.6, at a guess? [04:05] StevenK: Yeah [04:06] 2.6 eats the whole character [04:06] It assumes that the size indicated by the first byte is correct. [04:07] Hm, not sure how to deal with this test for both 2.6 and 2.7, then. :-( [04:07] http://bugs.python.org/issue8271 [04:12] wgrant: Ouch [04:12] wgrant: assertTrue(...['field.title'].startswith('...')) ? [04:12] StevenK: I'd just check if it's one of the two known values [04:13] With an XXX [04:13] Right [04:19] wgrant: http://pastebin.ubuntu.com/1595628/ [04:21] StevenK: Looks good [04:26] wgrant: -0 is waiting for stub, and I guess -1 can't land until after a FDT? [04:26] StevenK: Right. [04:26] Most of the fun will happen after an FDT at 1pm on Monday, I suspect [04:27] They're all in ec2 now [04:27] How many indicies do you add? [04:27] (the runs are condemned due to your failure, though) [04:27] Um, not sure at this point [04:27] That's for working on between 2.5 and 3 on Monday [04:27] Since I can approve them myself and get ops to apply them now [04:27] Last time I looked, your DB patch said TODO: add indicies [04:27] I removed that while you were reviewing the rest, I think [04:28] Ah [04:28] There's no point creating them at that stage [04:28] Quicker to create them at the end when all the data's there [04:28] Indeed [04:28] And it's not my failure, it's all 2.6's fault. [04:28] True [04:29] wgrant: Shall we deploy in about 70 minutes? [04:29] Might be closer to 80, but eh [04:30] So, remaining changes: work out indices, clean up all the _new crap, drop PackageBuild itself, drop the old DB columns/table, apply various NOT NULL constraints, work out indices for the new D/DS/SPN/i_d_a queries, integrate the new D/DS/SPN/i_d_a queries [04:30] And that should be that [04:30] And yes, we should deploy tonight [04:30] Will close 3 criticals? [04:31] Yah [04:31] I need to find another [04:40] wgrant: How can I pull apart this checkwatches core ? [04:41] You may not have much luck pygdbing it post-portem [04:42] post-mortem [04:43] Ideally we'd do it in-place [04:45] On loganberry? [04:46] Yes [06:45] stub: Thanks. Those indices were indeed planned for a quick followup, but the DEFAULT thing slip my mind this time. [07:16] yup, I assumed the indexes would be landing soon. Just making sure those particular ones don't get lost in case you were mainly thinking about performance tuning. [07:17] stub: Though it doesn't matter much for librarian-gc anyway [07:17] As it basically seqscans everything [07:17] But still useful [07:18] It's person merges that really matter, as they are selective [07:18] mmm... [07:18] I haven't looked for ages. I was thinking there was some selectivity in there. [07:19] Well, consider what it has to do :) [07:19] It needs to find LFAs that are unreferenced [07:19] Which means finding a list of everything that is referenced [07:20] The index only helps a lot if the column is relatively sparse [07:32] That is one part of the algorithm. There used to be parts that benefited from the indexes. But TBO, I don't really know if that is still the case after a few years of tweaking. [07:33] If nothing else, deleting a single row from LFA requires looking up if that row is referenced anywhere - index lookups or table scans if the indexes don't exist. [07:34] Which is hundreds or thousands each gc run [07:39] Oh yeah, that bit is a good point. [07:39] Duh === Masklinn_ is now known as Masklinn === yofel_ is now known as yofel === Ursinha is now known as Ursinha-afk === matsubara-afk is now known as matsubara === Ursinha-afk is now known as Ursinha === Ursinha is now known as Ursinha-afk === Ursinha-afk is now known as Ursinha === Ursinha is now known as Ursinha-afk === salgado is now known as salgado-brb === salgado-brb is now known as salgado === matsubara is now known as matsubara-lunch === slank_away is now known as slank === Ursinha-afk is now known as Ursinha === Ursinha is now known as Ursinha-afk === Ursinha-afk is now known as Ursinha === matsubara-lunch is now known as matsubara === deryck is now known as deryck[lunch] === Ursinha is now known as Ursinha-afk === Ursinha-afk is now known as Ursinha === mpt_ is now known as mpt === Ursinha is now known as Ursinha-afk === deryck[lunch] is now known as deryck === matsubara is now known as matsubara-afk === Ursinha-afk is now known as Ursinha === Ursinha is now known as Ursinha-afk