[04:42] hm [04:42] i don't suppose anyone is here?\ [10:01] * henninge is OCR but cannot change the channel subject anymore. Wonder why ... [10:13] henninge: likewise :-/ [10:13] intellectronica: they may have fiddled with the settings last week during UDW. [10:13] s/during/for/ [10:13] maybe [11:00] henninge: sorry for not having started with reviewing yet. i'm going for a fun little visit at the dentist's soon, but will be reviewing when i'm back, around lunchtime [11:01] intellectronica: I see. I will have to go out in the afternoon, too ... ;-) [11:01] henninge: cool, so at least we're doing shifts === mrevell is now known as mrevell-lunch [13:27] henninge: did you pursue the channel topic problem at all? we need to get that resolved. i can't change the topic in #launchpad either [13:28] bac: no, I didn't. I don't have any clue were to start. [13:28] me neither [13:28] s/were/where/ [13:28] i guess i'll try in #freenode [13:29] bac: I was able to change to topic on #launchapad-dev, btw. [13:30] me too [13:34] i'm seeing if our crack #is staff can help === mrevell-lunch is now known as mrevell [13:57] intellectronica: do you know why we projoin the tables Product and SourcePackageName in BugtaskSet.search()? [13:57] intellectronica: soory, wrong channel... [13:57] henninge: can you have a look at https://code.launchpad.net/~bac/launchpad/bug-515171/+merge/18385 ? it is super trivial [14:01] bac: r=me ;) [14:04] henninge: thanks === salgado is now known as salgado-lunch [15:15] EdwinGrubbs: ping [15:16] jamalta: hi [15:18] EdwinGrubbs: hi, i had noted in my calendar to bug you today about landing an MP.. do you have some time? [15:18] here's the link https://code.edge.launchpad.net/~jamalta/launchpad/relatedbugs-118609/+merge/18149 [15:19] jamalta: I'll try to land it. I'll let you know when it is in pqm. [15:19] EdwinGrubbs: thanks === matsubara is now known as matsubara-lunch [15:49] 1 2 3 test [15:50] intellectronica: tested [15:50] Ursinha: thanks :) [15:50] intellectronica: :) [15:51] Hello everybody. I've just pushed a very small branch that's ready for review: https://code.edge.launchpad.net/~andrea-bs/launchpad/fix-411007/+merge/18389 [16:03] andrea-bs: approved. Once it has passed the test suite, I will be happy to land it for you. [16:03] abentley: thanks! [16:05] abentley: oops! I've just noted that the branch has conflicts [16:06] andrea-bs, I don't see any conflicts in the text. I think the web page is bugged. [16:06] abentley: yes, there's a bug in LP. I'm going to report it === salgado-lunch is now known as salgado [16:09] well bac, in order to fix this I need to be an op here [16:09] so I'll need to get help for that [16:09] Rinchen: The channel isn't registered to begin with... [16:09] Rinchen: yeah, what jpds said [16:09] Rinchen: Hi! too. [16:09] jpds: yeah wasn't on the list when I was with LP to setup [16:09] Rinchen: Poke someone on: /who freenode/staff/* [16:10] Rinchen: i think people just create some channels without knowing there is a procedure to register them [16:10] * bac didn't know... [16:10] Rinchen: is the channel registered? [16:10] no Ursinha [16:10] Rinchen: if not, you can register and you're the op [16:10] then set guard mode on and give op to others [16:11] You can only register a channel whilst you *are* an op of it [16:11] maxb: you're right, if the channel has people on it... [16:12] that was easy :) [16:12] * Rinchen has connections [16:13] #launchpad-foundations is in need of the same treatment [16:20] bac: making progress but getting an error from chanserv setting privs [16:22] Rinchen, I am an OCR today, so I'll want to be able to change the topic, whether that requires ops or whatever. [16:22] abentley, bac - try oping yourselves please [16:22] Rinchen: /mode -t [16:22] Rinchen, it doesn't work yet. [16:22] * bac power! [16:23] super thanks bac [16:23] Rinchen: you did the hard work [16:23] I need to finish a comparison against #launchpad but it appears everyone should have the same access both places [16:23] Rinchen: did you change the mode to -t ? [16:24] intellectronica: could you try changing the topic here? [16:24] I had an error in the channel flags [16:24] you'll need to reop bac [16:24] bac: i am told that i am not channel operator [16:24] Same here. [16:25] intellectronica: I think you need to identify to NickServ. [16:25] * abentley is already identified. [16:25] intellectronica: please try now [16:25] to change topic [16:26] i am still not channel operator [16:26] intellectronica: the hash is set to those with @canonical or @freenode nicks [16:26] abentley: But no c/lp cloak. :-/ [16:26] oh, i see chanserv put it back to +t right after i did -t [16:26] jpds, been on freenode years before I was with Canonical. [16:27] damn [16:27] bac, maybe if you ask ChanServ to set the mode? [16:27] abentley: how? [16:27] jamalta: I'm running the full suite of tests even though it is unlikely that anything broke with that small change. You can see the progress here: http://ec2-174-129-125-7.compute-1.amazonaws.com/ [16:28] bac, I think the flags command would do it. [16:28] abentley: even though i'm an op it says i am unauthorized [16:28] Rinchen: /msg ChanServ set #launchpad-reviews mlock +nc-tslk [16:29] jpds: already done [16:29] jpds: as in, I removed it [16:29] Now -t should be removable. === matsubara-lunch is now known as matsubara [16:30] EdwinGrubbs: thanks for the update, will try and take a look at the link === jamalta is now known as jamalta-afk === jamalta-afk is now known as jamalta [16:46] anybody up for a review? [16:46] https://code.edge.launchpad.net/~al-maisan/launchpad/ejdt-n-1/+merge/18391 [16:59] jtv: ping [16:59] bac: hi [17:15] al-maisan: your mp reports conflicts [17:16] I can merge devel trunk w/o conflicts though [17:17] .. and the other way around: I can merge the branch under review into a fresh devel branch w/o conflicts [17:20] jtv, if there were actually conflicts, the affected files would be listed immediately below. [17:20] abentley: then what does it mean for the mp to say that there are conflicts? [17:20] yeah .. it's quite confusing [17:21] jtv, it means there's a bug, and that the web page treats anything that's not None as conflicts, including ''. [17:21] abentley: ah, known bug? [17:22] jtv, yes. [17:22] abentley: thanks for explaining. [17:34] al-maisan: minor æsthetic note: in get_builder_data, ll. 50—73, consider moving the computation of totals outside the loop... sum([count for ((processor, virtualized), count) in builder_stats.iteritems() if virtualized == True]) [17:34] Although if that's making things too complicated, nm. [17:34] jtv: let me take a look [17:36] good job catching what looked like an accidental interaction between "processor" and "virtualized" in the SQL composition code btw. [17:36] jtv: thanks :) [17:39] al-maisan: on another note, there is a way to stormify non-object queries ("SELECT count(*) FROM foo" and such) though I don't recall the syntax off the top of my head. [17:40] hmm .. hmm.. [17:40] Bad example; I meant things like "SELECT bar, count(*) FROM foo ORDER BY bar" [17:41] "GROUP BY"? [17:41] Sorry, yes. [17:46] al-maisan: for ll. 298—299, the head_job_platform query, can't you just write "return store.execute(query).first()"? [17:47] jtv: I guess so, that should be better. [17:47] oh, dinner gong === jtv is now known as jtv-eat [17:48] jtv-eat: enjoy your dinner :) [17:52] s/first/get_one/ [17:55] jtv-eat: http://pastebin.ubuntu.com/366981/ [17:56] * al-maisan is being called to dinner as well === EdwinGrubbs is now known as Edwin-lunch === jtv-eat is now known as jtv [19:14] al-maisan: in line 409, getEstimatedJobStartTime reads builder_stats[platform], where builder_stats comes out of get_builder_data. Is it absolutely certain that that entry exists in the dict? [19:14] jtv, just a second [19:14] jtv: yes [19:15] jtv: ..and if not a 0 will be returned [19:15] since I am using a defaultdict foir builder statistics [19:15] oic [19:15] *for [19:18] al-maisan: I wonder if maybe the normalization of head_job_platform should move into _getHeadJobPlatform. [19:21] jtv: I prefer normal functions if I can .. also the normalize_virtualization() function is used in quite a few places unrelated to the head job.. [19:22] al-maisan: but maybe this particular invocation would look better inside _getHeadJobPlatform()? [19:25] So imagine the whole "if is None" part would move into _getHeadJobPlatform. [19:25] * al-maisan looks at the code again [19:26] abentley: would you do an experiment and see if you can change the topic here? [19:26] * bac still wresting freenode === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [19:27] bac, yay! [19:27] abentley: cool, thanks [19:27] jtv: are you suggesting to move lines 182-185 into _getHeadJobPlatform()? [19:28] al-maisan: 181—185 even [19:28] And then you could probably do without line 204 and rewrite line 189 slightly [19:28] jtv: sorry .. that's what I meant to ask .. :P [19:28] :) [19:35] jtv: yeah .. I can make that change if desired. [19:36] al-maisan: thanks, my impression is it'll make for a cleaner composition. [19:40] no problem [19:41] jtv: I agree [19:48] Edwin-lunch: thanks so much, looks like pqm finished :) [19:52] al-maisan: next point... _freeBuildersCount sounds like you're freeing a builders count! [19:52] jtv: hmm .. I guess you're right .. it does sound like an action [19:53] jtv: how about _countFreeBuilders() ? [19:54] sure [19:54] .. or _getFreeBuildersCount() [19:54] fine [19:54] the latter is probably more conventional [19:55] yes, and avoids the notion that the counting itself is an action that you want the method to undertake. [19:55] It's the result that we're interested in. === Edwin-lunch is now known as EdwinGrubbs [19:55] hmm .. let's settle for _getFreeBuildersCount() then [19:55] Agreed. [20:01] al-maisan: the string on ll. 239—255 is doubly indented and the closing quotes are not indented far enough. [20:02] jtv: good catch .. I'll fix that. === matsubara is now known as matsubara-afk [20:06] al-maisan: in _getPendingJobsClauses, another way to express "buildqueue.virtualized = %(virtualized)s OR (buildqueue.virtualized IS NULL AND %(virtualized)s = TRUE" would be "COALESCE(buildqueue.virtualized, TRUE) = %(virtualized)s" [20:06] COALESCE returns the first of its arguments that is non-null [20:07] jtv: very cool [20:07] OR clauses can make things so ugly... :) [20:09] jtv: COALESCE is definitely more elegant :) [20:10] al-maisan: and another tip, more as background information: when composing SQL clauses like this, we often get more maintainable code by constructing a list of conditions and joining them with " AND ".join(conditions) [20:10] In this case it wouldn't matter much though. [20:11] jtv: yeah .. I agree, but in this case the clauses were all identical .. I was just looking to avoid repetitive code. [20:12] Right ho. === salgado is now known as salgado-afk [20:13] BTW the pattern of queries here suggests that you may be interested in the "DISTINCT ON" feature as well. It lets you say things like "give me only one buildqueue/job row per (lastscore, id) value, and ignore any rows with the same values that come later in the query's ordering." [20:14] (Again just background info, not trying to coerce you into using it here) [20:14] hmm .. I see. [20:17] al-maisan: in the docstring for getEstimatedJobStartTime, you mention "(weighted by the number of machines in the respective build pool)." Is the "weighted by" really the right term here? Not "divided by" or "distributed across" or somesuch? [20:17] "divided by" is probably better [20:17] thanks, that clarifies things for me [20:19] al-maisan: unnecessary parentheses on line 440 of the diff, in test_buildqueue.py. [20:19] jtv: noted, thanks! [20:19] That's an import. Or alternatively, the line needs to be broken up. :-) [20:20] removing parentheses is probably better [20:24] Yes, the choice depends entirely on line length [20:26] al-maisan: for assertEqual tests, we've standardized on passing the expected value first and the actual value second. Not asking you to chance all the existing ones, but maybe the ones you touched? [20:27] jtv: no problem -- I'll probably do all in one fell swoop [20:27] cool! [20:29] al-maisan: in test_buildqueue, you're disabling all native builders... is there no way to do that in a single loop without exhaustively naming architectures? [20:29] jtv: yeah .. should be doable .. will fix that. [20:30] thanks, I think i'll eliminate a distraction from the test. [20:44] jtv: I will be signing off now .. if there's anything else that needs doing please mention it in the review email. [20:44] al-maisan: sure, no worries [20:44] I'll try to live up to my fascist reputation :-P [20:45] jtv: ah, c'mon, admit that you like that ;) [20:45] al-maisan: oh YES I do! [20:45] links-zwo-drei-vier... [20:45] jtv: now it's on record :-) [20:46] jtv: thank you very much again for reviewing my branch and good night :) [20:46] al-maisan: well, at least nobody will accuse me of being *crypto*-fascist. :-) [20:46] oh, that's an advanced rank ;) [20:47] * al-maisan falls of the chair === abentley changed the topic of #launchpad-reviews to: on-call: - || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === jamalta is now known as jamalta-afk