=== Ursinha-bbl is now known as Ursinha-afk
=== adeuring1 is now known as adeuring
[11:33] <henninge> jtv: How about reviewing the gettext-check-messages branch in the sun?
[11:33] <henninge> https://code.launchpad.net/~henninge/launchpad/recife-gettext-check-messages/+merge/41155
[11:35] <jtv> henninge: sun?  It's night here.
[11:36] <henninge> just barely ... ;)
[11:36] <jtv> Completely dark.
[11:36] <jtv> Has been for a while now.
[11:36] <jtv> And that also means: eod!
[11:38] <henninge> jtv: you live by the sun? ;)
[11:38] <jtv> Sort of.  It's past EOD time.
[11:39] <henninge> oh well, I have to find me another reviewer, then.
[11:40] <henninge> allenap: are you reviewing today?
=== matsubara-afk is now known as matsubara
[11:51] <allenap> henninge: Strictly speaking, yes.
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing:  - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[11:52] <henninge> allenap: I think this is an easy one ... ;)
[11:52] <henninge> https://code.launchpad.net/~henninge/launchpad/recife-gettext-check-messages/+merge/41155
=== henninge changed the topic of #launchpad-reviews to: On call: allenap || Reviewing:  - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[11:52] <henninge> allenap: if you would be so nice, please?
[11:53] <allenap> henninge: Sure :)
[12:19] <henninge> allenap: I added a comment to give you a little more background.
[12:19] <henninge> I will be off to a longer lunch soon.
=== henninge is now known as henninge-lunch
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing:  - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[13:54]  * henninge is back
[14:15] <danilos> allenap, hi, I've got a nice branch for you :)
=== danilos changed the topic of #launchpad-reviews to: On call: allenap || Reviewing:  - || queue: [danilos] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[14:19] <danilos> MP with the diff up at https://code.launchpad.net/~danilo/launchpad/recife-translation-credits/+merge/41172
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: danilos || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== jaycee changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== jaycee is now known as jcsackett
[15:00] <gmb> jcsackett, allenap: Could I get a review for https://code.launchpad.net/~gmb/launchpad/bug-197775/+merge/41171 please?
[15:01] <jcsackett> gmb: i'll take it.
[15:02] <gmb> jcsackett: Manythanks.
=== jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos, gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[15:13] <allenap> Hello jcsackett, fellow review dude :)
=== salgado is now known as salgado-lunch
=== EdwinGrubbs changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos, gmb || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[15:42] <EdwinGrubbs> allenap, jcsackett: I put my branch in the queue: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-667900-dsp-page-upstream-link-form/+merge/41025
=== jaycee is now known as jcsackett
=== matsubara is now known as matsubara-lunch
[16:13] <jcsackett> gmb: comments are on your MP.
[16:13] <jcsackett> gmb: i mentioned it in the comments, but i will also need to ping sinzui to look at this, since i'm currently a trainee.
=== jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos, Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:16] <gmb> jcsackett: Thanks.
[16:16] <jcsackett> gmb: your welcome. :-)
=== salgado-lunch is now known as salgado
[16:21] <gmb> jcsackett: I've updated the branch. I'll request the review from sinzui
[16:21] <gmb> Thanks again.
[16:21] <jcsackett> gmb: you're welcome. thanks for those changes and grabbing sinzui.
=== Guest52674 is now known as jelmer
[16:30] <danilos> allenap, heya, I hope you are not having too much trouble with the review :) if you need any clarifications, I am around for on-call review as well
[16:44] <allenap> danilos: Sorry, I'm just being very slow and easily distracted :-/
[16:52] <abentley> allenap, jcsackett: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/find-review/+merge/41195 ?
=== jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || Reviewing: danilos, Edwin || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:52] <jcsackett> abentley: you're in the queue. :-)
=== deryck is now known as deryck[lunch]
=== allenap changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: Edwin || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== matsubara-lunch is now known as matsubara
=== benji is now known as benji-lunch
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== danilos changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: abentley || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[18:20] <danilos> jcsackett, hi, when you can get to it, one very simple branch up at https://code.launchpad.net/~danilo/launchpad/invalid-messages/+merge/41209 :)
=== deryck[lunch] is now known as deryck
[18:21] <abentley> jcsackett: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/builder-limits/+merge/41211 ?
[18:30] <danilos> abentley, isn't addressable memory used for other things than just actual memory? for instance, mmap files can take up a lot of AS
[18:30] <danilos> abentley, (I don't know much about RLIMIT_AS, so I am just wondering)
[18:32] <abentley> danilos: that is interesting, but we don't generally mmap things in bzr, and 1 GB is still huge.  Our example https://code.dogfood.launchpad.net/~abentley/+recipe/test/+build/4803 took 1 hour 28 minutes to fail, so I think there is lots of breathing room.
[18:35] <danilos> abentley, sure, I can see this is only a limit for recipe builders, but I wonder what'd happen if somebody tried to do things like language pack builds where source package itself is a few hundred megs (probably just like qtwebkit)
[18:36] <abentley> danilos: Also, our python doesn't provide RLIMIT_VMEM, so we don't have a lot of choice.
[18:36] <danilos> abentley, (though, this is unrelated to my first comment: I guess we want to fail early, so perhaps it's good anyway)
[18:36] <danilos> abentley, ok, it sounds good, but I wonder if we have a way to find out if we have been too aggressive
[18:37] <danilos> abentley, would we just track 'too many builds are getting killed because of this' or should we have something more specific in place?
[18:37] <abentley> danilos: We have lotsa logs.
[18:38] <danilos> abentley, I know, but I am sure we don't have a way to track these easily, and that's one thing I suggest: i.e. figure out a way to track these, especially right after it's rolled out
[18:38] <danilos> abentley, unless you count something like "grep SIGKILL buildd-manager.log" as "easy" :)
[18:38] <abentley> danilos: I view this as a necessary evil.  The current behaviour is catastrophic: https://wiki.canonical.com/IncidentReports/2010-11-17-buildd-manager-disabling-builders
[18:39] <danilos> abentley, yes, I agree, I am just thinking a bit more forward into "what if we killed too many builds that would have succeeded"
[18:39] <danilos> abentley, basically, I'm giving you my r=danilo, as long as we have some strategy in place to figure out that we were not too aggressive
[18:40] <danilos> abentley, i.e. something that will tell us later that 1GB was the right cut-off point (I trust your judgement in choosing it, it's just that it'd be nice to have a way to confirm it as a good choice later, when we can't do it now)
[18:40] <abentley> danilos: what would you consider an adequate strategy?
[18:41] <danilos> abentley, I don't know, a graph tracking number of builds failed because of this particular reason for instance, and a promise to look at it in say week's or two-weeks' time
=== benji-lunch is now known as benji
[18:42] <abentley> danilos: I don't know how to generate a graph of that.
[18:42] <abentley> danilos: You'd have to scrape the builder logs.
[18:43] <danilos> abentley, right, so is there a way to have this fail in a more specific way?
[18:44] <abentley> danilos: It's conceivable that there might be.
[18:44] <danilos> abentley, or, alternatively, at least a promise to do a one-time scraping of the logs so we know we haven't cocked up in significant way (if it's too serious we'll know it anyway, but what if we kill something like 15% of the builds that have worked in the past - how will we know?)
[18:45] <danilos> abentley, it doesn't have to be too formal, except that imo, it needs to happen
[18:45] <abentley> danilos: Well, a jump in "failed to build" here is a good hint: https://lpstats.canonical.com/graphs/CodeRecipeBuildsDailyStatusCounts/
[18:46] <danilos> abentley, ok, that's probably good enough if it's regularly tracked, even though it has quite some variation
[18:47] <abentley> danilos: We can certainly keep our eyes on it after the deployment.
[18:47] <danilos> abentley, that'd be great then, thanks, r=danilo
[18:57] <danilos> abentley, btw, can I ask you for a contra-review as well? it's removal of a method and related tests, all "red" :)
[18:57] <abentley> danilos: okay.
[18:57] <danilos> abentley, https://code.launchpad.net/~danilo/launchpad/invalid-messages/+merge/41209
[18:59] <danilos> abentley, the only tricky bit is at the very end, but that was a work-around introduced for the getPOTMsgSetWithErrors tests to pass (we didn't want to worry about it at the time to keep branch size in check, so this is a separate branch for this)
[19:01] <abentley> danilos: r=me
[19:02] <danilos> abentley, thanks
[19:37] <jcsackett> abentley: i apologize for a long delay on this review. "stepping out for lunch" turned into "oh god i have locked myself out of my workspace." :-(
[19:37] <abentley> jcsackett: Ah.  No worries.
[19:38] <jcsackett> i have few questions (for my own edification--i don't believe they constitute issues with the branch).
[19:38]  * benji is amused by jcsackett's misfortune. ;)
[19:39] <jcsackett> abentley: the aim of this branch was to move the api exposure of getMergeProposal to IBranch, correct?
[19:39] <jcsackett> (in order to be able to hunt down MPs over the api)
[19:39] <abentley> jcsackett: the main aim was to expose merged_revno.
[19:40] <abentley> jcsackett: Other IHasMergeproposal objects will still have getMergeProposal, so it's not "moved", it's extended.
[19:40] <jcsackett> abentley: ah. okay.
[19:43] <jcsackett> abentley: some other questions (likely dumb ones--i've never seen much of this code before).
[19:44] <jcsackett> abentley: the use of patch_collection_return_type is necessary just so the webservice is aware of precisely what collection it's getting?
[19:46] <abentley> jcsackett: Yes.  Normally, we'd have IBranchMergePoposal in the definition, and we wouldn't need it.  But because interfaces.branchmergeproposal imports interfaces.branch, we can't import interfaces.branchmergeproposal into interfaces.branch.
[19:46] <jcsackett> ah, which is why the return type decorator on the method says Interface.
[19:46] <jcsackett> okay, thanks. that makes sense.
[19:57] <jcsackett> abentley: i see a change to getMergeProposals on GenericBranchCollection to include the merged_revnos argument, but no corresponding change to the IBranchCollection interface it implements. isn't that required?
[19:58] <abentley> jcsackett: no, the tests pass without it.  It's only required if we have a security-proxied object, and we don't.
[19:58] <jcsackett> abentley: given the docstring says to refer back to the interface though, isn't awkward to have the interface definition differ?
[19:59] <jcsackett> is there a reason to not add it?
[19:59] <abentley> jcsackett: The reason not to add it is that it only makes sense to use when you use it the way Branch uses it.
[20:00] <jcsackett> abentley: fair.
=== Ursinha is now known as Ursinha-afk
=== jelmer is now known as Guest67633
[20:20] <EdwinGrubbs> sinzui, jcsackett: here are screenshots of the upstream portlet with the summary. I don't think including the logo is beneficial, and it is more useful to see the PROJECTGROUP=>PROJECT=>SERIES info, which has the icons. https://devpad.canonical.com/~egrubbs/upstream/
[20:22] <jcsackett> EdwinGrubbs: i like the addition of the summary, though i wonder what it might look like with a really long summary. not sure there's a good solution for that. :-/
[20:22] <sinzui> EdwinGrubbs, I like the summary. The icon is fine
[20:22] <jcsackett> i agree with icons over the enormous logo.
[20:22] <sinzui> jcsackett, the project owner (Us in 1100 cases should set a single sentence for the summary. It is for search listings)
[20:23] <jcsackett> sinzui: dig.
[20:27] <sinzui> EdwinGrubbs, you have my r=me
[20:27]  * sinzui find mp
[20:28] <EdwinGrubbs> sinzui: thanks
[20:30] <jcsackett> abentley: r=me. i had some comments on the MP, but they're not blockers in my opinion. since i'm training on this, sinzui will have to follow up. i've already requested a review from him.
[20:30] <jcsackett> sinzui, to save you time: https://code.edge.launchpad.net/~abentley/launchpad/find-review/+merge/41195
[20:30] <jcsackett> danilos: abentley reviewed the MP you put up for the queue already, didn't he?
[20:30] <danilos> jcsackett: that's right, though I'll shortly have another one :P
[20:30] <abentley> jcsackett: yes.
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[20:31] <jcsackett> danilos: i'll happily review that when it's ready. :-)
=== salgado is now known as salgado-afk
[20:33] <sinzui> abentley, r=me
[20:33] <abentley> sinzui: thanks.
[20:34]  * jcsackett is fast learning reviewing really is as much about educating the reviewer as anything.
[20:41] <allenap> jcsackett: Fancy another learning opportunity? ;)
[20:41] <allenap> https://code.launchpad.net/~allenap/launchpad/subscribe-to-tag-bug-151129-2/+merge/41186
[20:45] <abentley> jcsackett: Yes, reviewing is primarily an education exercise.  Frequently in both directions.
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[20:49] <jcsackett> allenap: looking at it now.
=== danilos changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: allenap || queue: [danilo x 2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[20:51] <allenap> jcsackett: Thank you :)
[20:54] <danilos> jcsackett: damn, allenap beat me to it :) anyway, two very simple branches in the queue
[20:55] <danilos> first one is https://code.launchpad.net/~danilo/launchpad/bzr-export-pofile-gathering/+merge/41227 and next one is https://code.launchpad.net/~danilo/launchpad/bug-669831/+merge/41228
[21:00] <allenap> danilos: Sorry dude :)
[21:00] <jcsackett> danilos: happy to look at 'em. may take a bit, allenap's appears vast. :-P
[21:01] <danilos> jcsackett: I hope you don't mind me dropping off then? they should be really simple and short (one is ~140 lines with 10 lines of moved code and tests added for that, another is 2 line diff with another test added for it :)
[21:01] <danilos> or perhaps allenap can help a poor fellow like me who wants to head home :)
[21:02] <jcsackett> if allenap wants to take a peak, that's cool. if not, it's fine, i'll leave comments on the MP if i have any.
[21:02] <jcsackett> danilos^
[21:02] <danilos> jcsackett: cool, that should be fine
[21:02] <jcsackett> allenap: any reason that you inline import structuralsubscriptionfilter? is there a circular problem you can't work around?
=== Guest67633 is now known as jelmer
=== matsubara is now known as matsubara-afk
[21:06]  * danilos wanders off...
[21:18] <allenap> jcsackett: Yes, it's a circular import problem.
[21:18] <allenap> I'll have a look at danilio's branches. Missed the pings.
[21:19] <jcsackett> allenap: ah, thanks.
[21:20] <jcsackett> for both the answer and looking at danilos's stuff.
[22:08] <allenap> jcsackett: How's it going? Got any questions? Unfortunately I have to toddle off soon, but if you have any questions later you can ask them via the merge proposal.
[22:08] <jcsackett> allenap: i'm doing alright. there was a lot of code to read through (those filters). :-)
[22:09] <jcsackett> i'm about to post to the MP.
[22:14] <jcsackett> allenap: comments are on MP. :-) sorry for the wait.
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[22:18] <allenap> jcsackett: Cheers, that's awesome :)
=== jcsackett changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews