#launchpad-reviews 2010-05-24
<thumper> trivial review for someone: https://code.edge.launchpad.net/~thumper/launchpad/vcs-imports-permission-review/+merge/25867
* intellectronica changed the topic of #launchpad-reviews to: On Call: intellectronica  || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> intellectronica: hi. is there anything I need to do for having this branch landed ? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-570899/+merge/25443
<intellectronica> adiroiban: no, i'll submit it for tests and landing on your behalf
<leonardr> intellectronica: i have a long (850 lines) but hopefully stimulating branch for you to review
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/representation-cache/+merge/25895
<intellectronica> leonardr: cool. let me just finish a smaller branch i've started and i'll do yours
<leonardr> great
<intellectronica> leonardr: also, can i interested in reviewing a (much smaller) branch of mine?
<leonardr> intellectronica, sure
<intellectronica> leonardr: cool, let me just create an mp
<intellectronica> leonardr: https://code.edge.launchpad.net/~intellectronica/launchpad/expectations-bug-556499-model/+merge/25896)
<intellectronica> leonardr: very nice improvement. looks good to me. r=me.
<gary_poster> leonardr: in your branch, when there is no cache, did you contemplate always generating it, even if there are redacted fields?  Example: if no cache, generate dict of the entire non-redacted version; else if cache and redacted fields, parse out cache to dict; else return cache.  (Now we have a non-redacted dict, if we are still here.)
<gary_poster> Now, redact dict, turn into JSON, and return.  There are variations of that, some of which might be better, but I imagine you get the drift.
<gary_poster> (looks nice to me too :-) )
<leonardr> gary: thinking about your comment
<gary_poster> cool
<leonardr> gary, i'm not sure what the benefit would be
<leonardr> also, if there are redacted fields we _cannot_ calculate an unredacted cache due to the security policy
<gary_poster> leonardr: the goal would be to create a source for further cache hits.  This could be particularly important for objects that frequently have one or more fields redacted.  In that case, the cache would rarely or, in the worst case, never be filled (and therefore never or rarely used).  Since DB access is the main expense, you discovered, I strongly suspect that loading JSON and redacting will be significantly cheaper tha
<gary_poster> creating the JSON.
<gary_poster> Also, I'm skeptical of "cannot"; isn't it just a matter of doing the usual work with an unproxied object?
<leonardr> yes, we would have to strip the proxy
<leonardr> ok, i see what you're saying. we would cache it all the time, whether we were sending a redacted version or not
<gary_poster> right
<leonardr> i could certainly do that in a future branch. do you know of launchpad objects that typically have redacted fields?
<gary_poster> bac would probably know, but he's out.  My first guess: anything private, or (perhaps more interesting, perhaps not) anything referring to something provate.
<gary_poster> private
<leonardr> if an object's url contains private information, a link to that url would be redacted
<gary_poster> so, that's an example?
<leonardr> but i don't know of any specific launchpad object that does that. it's something to look for
<gary_poster> bugs that are marked as security issues
<gary_poster> private projects
<gary_poster> private teams
<gary_poster> private bugs
<leonardr> so anything that links to those objects might end up redacted
<gary_poster> (and there's more coming, if I understand correctly)
<gary_poster> right
<leonardr> ok, let's get the basic cache working, make sure it improves performance in real situations, and then i'll work on that
<gary_poster> cool, makes sense
<adiroiban> danilos: I have added a comment for bug 583979 . Maybe we can have the pre-implementation chat now. What do say ? Do you have time?
<leonardr> intellectronica: r=me, sorry for the delay
<intellectronica> leonardr: thanks!
<rockstar> intellectronica, are you still reviewing?
<intellectronica> rockstar: yeah. got my review?
<intellectronica> any questions?
<rockstar> intellectronica, do I owe you a review?
<intellectronica> i can't start on a new branch, though
<intellectronica> rockstar: no, but i reviewed a branch of yours and requested a change
<rockstar> intellectronica, oh, you did?
<rockstar> intellectronica, where am I using an inline javascript event handler?
<intellectronica> rockstar: onclick="return somethingSeomthing..."
<rockstar> intellectronica, oh, that's an artifact of old code.  I just moved it, and I'm going to YUI-ize that soon.
* intellectronica 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
<rockstar> intellectronica, I'd move to not make any changes to that in this specific branch, but I'm happy to file a bug about it.
<intellectronica> rockstar: ok, if you're aware of it and have plans to imrpvoe it later i'm fine with it landing like this
<rockstar> intellectronica, I have a branch right now that entirely re-does the markup of the branch index page in general.
<intellectronica> rockstar: cool. make a comment to that effect in the mp and i'll approve it
 * intellectronica sheds a tear as he ends his last ever on call review shift
<rockstar> intellectronica, done.
<rockstar> intellectronica, is this your last week?
<intellectronica> rockstar: it is
<rockstar> intellectronica, :'(
* sinzui changed the topic of #launchpad-reviews to: On Call: -  || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-05-25
<rockstar>  thumper around?
<thumper> aye
<rockstar> thumper, can I get a code review for https://code.edge.launchpad.net/~rockstar/launchpad/remove-sourcepackage-from-recipe/+merge/25809 ?
<rockstar> It has it's two db reviews
<thumper> ok
<thumper> rockstar: done
* henninge changed the topic of #launchpad-reviews to: On Call: henninge  || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On Call: henninge, gmb  || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> Hi henninge.
 * gmb keeps forgetting he's on the rota again 
<henninge> Hi gmb! ;)
<henninge> bac and I never finished that conversation about changing my slot, so I am still here ;-)
<gmb> :)
* sinzui changed the topic of #launchpad-reviews to: On Call: henninge, gmb  || reviewing: sinzui || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> henninge, gmb, I just added a trivial branch the to review queue
<gmb> sinzui, I'll take a look shortly; otp atm
* gmb changed the topic of #launchpad-reviews to: On Call: henninge, gmb  || reviewing: sinzui, sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> sinzui, The over-and-over branch, right?
<sinzui> gmb, yes
<gmb> sinzui, r=me
* gmb changed the topic of #launchpad-reviews to: On Call: henninge, gmb  || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> fab
<adeuring> gmb, henninge: could you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-511269/+merge/25971 ? (sorry, more than 900 lines...)
<gmb> adeuring, Sure, I'll take a look
<adeuring> gmb: thanks!
* gmb changed the topic of #launchpad-reviews to: On Call: henninge, gmb  || reviewing: sinzui, adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> sinzui: review sent
<sinzui> thank
* henninge changed the topic of #launchpad-reviews to: On Call: gmb  || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> adeuring, r=me; nice work!
<adeuring> gmb: thanks!
* gmb 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
<adeuring> gmb: could you please update the MP?
<gmb> adeuring, Bugger. Yes, sorry.
<adeuring> gmb: ;) thanks!
<gmb> adeuring, Done.
<adiroiban> danilos: did you have time to take another look at the POTemplate API branch and send it to ec2-test?
<danilos> adiroiban, sorry, no
<adiroiban> danilos: ok. and will you have time to discuss the pre-implementation details for bug 583934 and bug 583979 ? Not now, but before the start of June :)
<mup> Bug #583934: API for POFile attributes <api> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/583934>
<danilos> adiroiban, yeah, I should have some time later this week
<adiroiban> danilos: ok. just poke me when you have time . For both bugs I have added comments or pushed/attached a branch containing some ideas of how I see this can be done.
<mars> abentley, ping
<danilos> adiroiban, sure
<mars> abentley, unping
#launchpad-reviews 2010-05-26
<gary_poster> mwhudson: ping?
<mwhudson> gary_poster: hi
<gary_poster> hey mwhudson .  mars asked me to review a branch in lieu of a domain expert.  Do you feel domain expert-y about process groups and various signal processing?  If not, I'm happy to crack open my UNIX programming tome and pretend to be a domain expert.  Branch is https://code.edge.launchpad.net/~mars/launchpad/fix-test_on_merge-578886/+merge/25981 fwiw.
<mwhudson> gary_poster: maybe a little bit
<gary_poster> mwhudson: :-)
<mwhudson> (also, i'm currently using APUE as a monitor stand :)
 * mwhudson looks at the branch
<gary_poster> LOL
<gary_poster> thank you very much for looking at it mwhudson!  If you don't get around to it, no worries.  I'll pick it up tomorrow.
<mwhudson> gary_poster: i reviewed the branch
<mars> mwhudson, I used the Windows 2000 System Administrator's guide at my last job.  two books, 4 inches thick - perfect for two cubicle monitor stands.
<mars> mwhudson, thanks for the review
<mwhudson> mars: i use two books, the other one is "xUnit test patterns"
<mars> that's on my to-read list
<mars> reading Release It! right now
<mwhudson> mars: if i'd ever owned any windows system administrator's manuals, i'd have gotten rid of them when i moved to nz :)
<mwhudson> (as i did for /most/ of my books)
<mars> mwhudson, have a sec for a question about that review?
<mars> just want to be clear on the process group leader problem you mentioned
<mwhudson> mars: sure
<mars> mwhudson, ok, so you are worried about line 58, "assert original_process_group != os.getpid()", right?
<mwhudson> mars: yes
<mars> ok
<mwhudson> mars: in your branch if you execute ./test_on_merge.py in a shell, it will build the schema, then fail on that assert
<mars> mwhudson, so I don't understand the usePTY=True bit.  What is that setting, and what would it do?
<mwhudson> mars: it's a buildbot thing
<mwhudson> if you use it buildbot runs your subprocess with a pseudo tty attached
<mwhudson> and, thanks to a twisted api oddity, in a new session (in the setsid() sense of session)
<mwhudson> which implies creating a process group
<mars> wow, that's arcane
<mwhudson> yeah, it is a bit
<mars> how did you find that out?
<mwhudson> google
<mwhudson> specifically http://permalink.gmane.org/gmane.comp.python.buildbot.devel/5680
<mars> I think I meant "what prompted you to discover this particular arcane bit of knowledge" :)
<mwhudson> mars: review paranoia ... "i wonder if buildbot starts processes in their own process group"
<mars> "Also, there's the obscure case where buildbot runs a process that processes that fiddle with their process group, and that would this feature"
<mwhudson> mars: i thought there was a chance it would do it always
<mars> well, guess what make -> test_on_merge.py -> bin/test were doing before!
<mwhudson> oh right
<mwhudson> yes
<mwhudson> i think the shell use case is more compelling tbh
<mars> sorry?
<mwhudson> i think not being able to type ./test_on_merge.py in a shell would be bad
<mars> I can't say for sure on that one.  How would the python paths get set up if you did not use bin/py to start it?
<mars> _pythonpath is supposed to be a bridge [hack]
<mwhudson> mars: well, ./bin/py test_on_merge.py fails too
<mars> oh.  Eww.
<mwhudson> mars: the reexec could be done with bin/py i guess
<mwhudson> i guess you could fork() and have the parent just waitpid on the child, that would have the same effect i guess
<mars> mwhudson, so what would the re-exec do?
<mars> yes, fork() would work nicely too
<mwhudson> mars: get us into a situation where we're not the process group leader
<mars> gives you the new PID you need for the process group swapping
<mwhudson> perhaps fork would be cleaner
<mwhudson> (man unix is crazy)
<mars> windows is not fun either.  Unix is just layers from different eras (System V, BSD, X, Free Desktop).  Windows is a new revolution every 5 years that obsoletes everything you learned before.
<mars> at least, that has been my experience with writing application software
<mars> mwhudson, interesting idea, but I think the fork() may be more understandable.  I find that spawnl() line suspicious.  What if you used bin/py?  Then you don't want the "-S" argument.
<mwhudson> i don't know that there is much excuse for job control to have invaded the unix kernel as much as it did
<mwhudson> mars: i think fork() would be preferable too, it hadn't occurred to me when i wrote my version
<mars> I wondered why I have to do this at all.  Python has no way to pull the process tree without running 'ps' and parsing the output.  Otherwise I would have just walked and killed the tree myself :/
<mwhudson> mars: although, sys.executable will never be ./bin/py so that particular objection isn't actually valid
<mwhudson> mars: i think that kind of thing is one of those horrible unportable unix things isn't it?
<mwhudson> though these days just supporting linux and os x would be fine
<mars> mwhudson, so you run it with bin/py, then it re-execs itself with sys.executable without your permission... that could be annoying.
<mars> that is another reason to prefer fork()
<mars> mwhudson, is there anything that has to be done when executing the fork() beyond... os.fork(), check if child?
<mwhudson> mars: bin/py ends by execing with its sys.executable
<mars> ah
<mwhudson> but this is all silly, fork() is better
<mwhudson> mars: no, i don't think so, pid = os.fork(); if pid != 0: os.waitpid(os.P_WAIT, pid)
<mwhudson> i think?
<mars> no messing with TTYs or streams or whatever?  Just remembering the hassle to correctly fork a daemon...
<mars> maybe File descriptors - daemonizing requires you to mess with file descriptors
<mwhudson> i think because the parent process isn't doing anything we're ok
<mwhudson> it would be worth checking that control-c still stops the test run
<mwhudson> (but i think it will, that much worked with the spawnl version)
<mars> hmm, that is something else I did not check
<mars> how EINTR is handled in my script
<mwhudson> i guess we could close stdin in the parent, but if ./test_on_merge tries to read from stdin the situation is kinda messed up anyway
<mars> :D
<mars> mwhudson, so should it fork always, just for simplicity's sake?
<mars> I like simplicity here
<mwhudson> yes that makes sense
<mars> actually that makes is much simpler
<mars> no more shenanigans then
<mars> just os.fork(), os.setpgrp()
<mars> oh, wait
<mars> nope
<mars> still need the shenanigans
<mars> mwhudson, the problem before was that bin/test would lead the process group.  test_on_merge.py would kill the entire process group of it's child process, and that worked up until xvfb got stuck in between
<mars> mwhudson, xvfb shares test_on_merge's process group, so os.killpg(9) ....
<mars> well, we'll see what looks nicer.  I could rewrite the code either way, with the forked child or the main thread doing the group cleanup.
<mars> mwhudson, thanks for the review.  I'll write some code.
<mwhudson> mars: i don't know _why_ control-c ing test_on_merge works in my branch, only that it does :)
<mars> try using a test harness that ignores SIGTERM, then you might see the problem
<mwhudson> mars: backtracking a bit,
<mwhudson> <mars> just os.fork(), os.setpgrp()
<mwhudson> is precisely not what we want: it's what the shell did
<mwhudson> and we don
<mwhudson> 't want to follow the fork with a setpgrp(), that's the point
<mars> but wasn't the point of forking to get a new PID, so you could do the process-group swapping trick and use that new PID for the child process's process group?
<mars> s/"child process's group"/xvfb-run prcoess group/
<mwhudson> mars: the process that invokes xvfb-run needs to not be the process group leader, so it can switch to being process group leader and back again
<mwhudson> i think?
<mars> mwhudson, correct.
<mars> mwhudson, well, technically, the process that invokes xvfb-run needs to switch it's process group to something safe, start xvfb-run, then switch back.  Since the only convention for creating a new process group is to use our own PID, then our own PID must not already be in use (in use because we are already the group leader)
<mwhudson> mars: right
<thumper> mars, mwhudson: can one of you approve this one ? https://code.edge.launchpad.net/~thumper/launchpad/send-mail-job-logging/+merge/25939
<mars> thumper, doing PDR, maybe mwhudson can take it?
<mars> it is already really late here
<mwhudson> thumper: done
<thumper> ta
<stub> https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/26017 <- should be a quick one
<noodles775> Hi stub, I'm about to test my (somewhat large) db-patch on dogfood, but just had a question about contingency for the patch if you've time to comment: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-changes-build-generalisation-new/+merge/25594
<stub> noodles775: Your testing on dogfood so we don't need contingency patches.
<noodles775> stub: well, we'd like to get df back to its normal state afterwards (without having to re-import the whole db), but I was wondering more whether for production there is any way back.
<stub> Not really - once it is in production, the old data becomes out of date.
<stub> And it won't be there anyway, as the table will be dropped by the upgrade procedure :)
<noodles775> OK, the last point was the bit I was after (or whether it's worth having a patch that will re-create the old table from the new).
* noodles775 changed the topic of #launchpad-reviews to: On Call: -  || reviewing: - || queue: [stub, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On Call: jtv  || reviewing: - || queue: [stub, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On Call: jtv || reviewing: stub || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> stub: might as well drop the "set the pageid in the WSGI environment" comment to the request.setInWSGIEnvironment('launchpad.pageid', pageid) comment.  Bit redundant.  :)
<stub> k
<jtv> stub: And at the very end of the diff, where you parse a '-' record, I'd extract a function so you don't obscure the elif block.  This is pretty close to the code structure that broke the worldwide AT&T network in 1992.  :-)
<jtv> (That was a "break" that used to be in a loop suddenly breaking out of a "switch" insteadâin this code the equivalent is a "continue" in an "elif" sequence)
<stub> k. I can extract it to a local function so I can still access the same scope.
<jtv> Yes, require_args could be a bit painful.  Frankly I don't see the value of making it local.
<stub> I think the new version is less readable
<stub> I can get rid of the continue if you don't like it using two if's instead of one (if record_type == '-' and len(args) == 1 etc.)
<stub> That seems to work and is clearer anyway
<stub> And the extension record parsing breaks out nicer too
<stub> Oh poo... mixed it up with uncommitted changes
<jtv> stub: sorry, distractions... back in a bit
<jtv> stub: whatever works...  though I do feel that break/continue should never be separated from the loop primitive by anything other than a surrounding "if" and possibly a preceding block of "if foo: break/continue" clauses.  Anything beyond that and you're probably simulating a return from a smaller function.
<jtv> The nice thing about "return" is that all flows of control come back together so unambiguously.
<stub> Pushing new version now. I think this meets all your criteria
<jtv>  diff still updating...
<stub> jtv: Done
<jtv> stub: welcome back... looks like we both got cut off despite different providers.
<jtv> stub: I was just asking about tests.
<stub> There aren't any
<jtv> Yes, that part was clear to me.  :)
<stub> Rationalization is that we don't know what the report looks like yet
<jtv> That'd make things harder, yes.  Would it be possible at least to feed a few bits of text into the parser to illustrate bits and pieces, and make sure the parser doesn't just bomb out completely?
<jtv> Beyond that, we don't want to chase after performance problems that are reporting artifacts, like the weather networks have been dropping the occasional minus from temperatures.  :-)
 * jtv is over-using smileys to hide a mean mood.
<stub> Maybe tomorrow. That branch went up 'as is' as something more urgent came up. Looks simpler than I thought so I might get that finished today though.
<jtv> Cool.  I'll make a note.
* jtv changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [noodles775, gmb(http://is.gd/cpL9M)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> adiroiban, btw, we'll have to change property names on bug 525371 at least
<mup> Bug #525371: API for reading POTemplates attributes <api> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/525371>
* gmb changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> danilos: what are the new names ? :)
<danilos> adiroiban, i.e. things like "all_potemplates" should be "translation_templates", and "all_pofiles" should be "translation_files"
<danilos> adiroiban, "potemplate" name is a "gettextism" and we are gradually getting rid of those
<adiroiban> danilos: ok. I was using the other properties like âall_milestonesâ
<adiroiban> as guidelines for naming
<danilos> adiroiban, not at all :) the problem is that I haven't had enough time to properly consider all this
<adiroiban> ok
<danilos> adiroiban, you are coding too fast :P
<adiroiban> then âall_translation_templatesâ ?
<danilos> adiroiban, no, not "all"
<adiroiban> ok
<danilos> adiroiban, what I don't like either is that we are getting a mixture of unclear interfaces that our objects implement, and that's something we should fix
<danilos> adiroiban, it's not something that can be done without a full picture of where we are going, so I have to pause a bit and reconsider what we have today and what we want to have
<adiroiban> danilos: ok.
<adiroiban> danilos: then I will stop the work on the API and will wait further orders
<danilos> adiroiban, no, that's ok, we should be able to push this one forward, and that will give us a better idea of where we want to go
<adiroiban> danilos: since I am new to Rosetta developement and I new little about your future plans
<adiroiban> it is hard for me to go allong and implement exactly what you want
<danilos> adiroiban, not to worry :)
<adiroiban> danilos: so what do you suggest I should do next ? (beside getting rid of gettext like names )
<danilos> adiroiban, it's not your fault at all: I did a review of your branch, but I did it badly... for instance, why do we need both "getTranslationTemplates" and "all_translation_templates"?
<danilos> adiroiban, we are simply introducing a lot more inconsistency, and we should not let that to happen; so, we should go over the interfaces one by one
<adiroiban> danilos: getTranslationsTemplates was the old interface attribute
<adiroiban> danilos: but lazr.restful can not export a method as an attribute
<danilos> adiroiban, right, no reason to not drop it, I guess? if it's not really used anywhere
<adiroiban> danilos: this is why I had to add a new property to export that method
<danilos> adiroiban, right, but that taints the interface unnecessarily: if it can be a property, it should be a property
<danilos> adiroiban, if it can't be a property it should be a method (both exported and internal)
<danilos> adiroiban, some reasons behind some of these interfaces are performance, and they are sometimes harder to grasp
<adiroiban> danilos: ok. then I will have to file a bug for each such property
<danilos> adiroiban, well, why do you believe this shouldn't be exported as a method?
<adiroiban> danilos: for example IRosettaStats is full of messageCount(self, langauge=None)
<danilos> adiroiban, IRosettaStats needs serious refactoring
<danilos> adiroiban, it's totally, utter, crack
<adiroiban> danilos: and in most cases the that method just returns the cached data from the table
<danilos> adiroiban, which is exactly what we want most of the time
<danilos> adiroiban, but, the problem with IRosettaStats is that it represents two different interfaces (language=None? wtf?)
<danilos> adiroiban, the names of the properties are totally outdated on it as well
<danilos> adiroiban, but anyway, I don't want to dwell on IRosettaStats in particular
<danilos> adiroiban, it's an interface that needs to go out, basically
<adiroiban> danilos: yes, IRosettaStats is a mess and that language=None is only implemened by IPOTemplate in a very strange way
<danilos> adiroiban, that strange way is called "broken" way :)
<adiroiban> danilos: the idea is that by exporting a data as an API attribute, we can get all attribute in one go
<danilos> adiroiban, not sure what you mean? you can't do that with a method?
<adiroiban> danilos: otherwise, it will require many API method calls that will just return cached data... and that data is already available (no need for intense queries)
<danilos> adiroiban, API should *always* return cached data
<adiroiban> danilos: for API attributes, or for both data returned by attributes and methods
<danilos> adiroiban, for anything that's expensive to calculate, if we have cached data, that's what we should return
<adiroiban> danilos: ah. but if we don't have cached data ? should we generate a  cache for the API, or generate the data on the fly ?
<danilos> adiroiban, if we don't have cached data, we should calculate it if it's cheap; if it's not cheap, we should think twice before exporting it
<adiroiban> danilos: ok :) back to IPOTemplate
<danilos> adiroiban, right :)
<danilos> adiroiban, so, calculating a list of templates is a relatively cheap operation on the database side; it's probably not that cheap on storm side
<adiroiban> danilos: so you suggest that instead of calling âubuntu/hoary/translation_templatesâ we should implement it as a method and call âubuntu/hoary?ws.op=getAllTemplatesâ ?
<danilos> adiroiban, it does make more sense, doesn't it? i.e. you'd be able to change the parent object in the script and call the same method
<danilos> adiroiban, sure, the same holds for the property, but there's no reason to introduce two things which basically do the same thing
<adiroiban> danilos: you can change the parrent and call the same attribute
<danilos> adiroiban, the important thing that I don't know an answer to is how does API support slicing
* bigjools changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [noodles775, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> danilos: from what I noticed in the other exported API data, if a full list is exported using the API, then it is exported as an attribute
<adiroiban> danilos: if you want to export a subset, then a method is used
<danilos> adiroiban, the examples on https://dev.launchpad.net/API/ImplementingAPIs for collections are pretty good
<danilos> adiroiban, not really, the example in the above page talks about active_members which is a subset of all members, and that's a property
<danilos> adiroiban, I think that "full" vs "subset" isn't, and shouldn't be a criterion to decide upon
<danilos> adiroiban, it should be the sanity of API
<adiroiban> danilos: like ubuntu/hoary/milestone will return a list of all milestones, while ubuntu/hoary?ws.op=getMilestiones&name=SEARCH_TEXT will return the milestones mathching that name
<danilos> adiroiban, so, my take is: if what we usually need is all translation_templates, then that should be exported as "translation_templates"; but if what we usually need is current translation templates, that should be exported as translation_templates
<danilos> adiroiban, one example is not conclusive, it's just an example
<danilos> adiroiban, "adminmembers" on ITeam is a counter-example
<adiroiban> danilos: true.
<danilos> adiroiban, also, we should seriously consider not snapshooting such big exports
<danilos> adiroiban, (I am worried about ~1500 templates on ubuntu distroseries being snapshotted)
<adiroiban> danilos: so how you do you suggest we should export the templates attached to a series(entity) ?
<danilos> adiroiban, I am not totally sure
<danilos> adiroiban, properties are nicer because we don't have to add person parameter :)
<adiroiban> danilos: how can we avoid snapshoting those 1500 templates. Just asking as I don't know exact why and how this snapshoting mecanism works
<danilos> adiroiban, well, snapshotting can be disabled by the use of doNotSnapshot, and what it does otherwise is fetch all objects from the database and serializes them
<adiroiban> danilos: and if we use doNotSnapshot, it will just get the total count of entires and a batch/slice of them (like 50 or 300 templates)
<adiroiban> ?
<danilos> adiroiban, I don't know :)
<danilos> adiroiban, that's what I am wondering as well
<adiroiban> danilos: ok. I could not find anything regarding âsnapshotâ on dev.lp.net, but I will look into the source code
<adiroiban> danilos: we had the same problem with snapshot when we exported all languages from Launchpad
<danilos> adiroiban, I am pretty sure what it does is instead return only links to objects which you have to load then
<danilos> adiroiban, but, it's more interesting to know if it batches or not
<adiroiban> danilos: do I guess we should also inform the API to not create a snapshot for the templates... as it looks like the implicit behaviour is to serialize all the results
<adiroiban> s/do/so/
<danilos> adiroiban, i.e. if default batch size for collections is 150 (as wiki page suggests), then saving on serialization of other 1300 is a nice enough win
<danilos> adiroiban, yeah, most likely, though that will make API harder to use: i.e. what we actually care about the templates is their stats directly, so perhaps we could snapshot those at least
<danilos> adiroiban, i.e. doing a single query to get all templates and then walking over each one separately is going to be very slow
<danilos> adiroiban, but, we'd have to think about what's the best way out of it
<danilos> adiroiban, anyway, first thing first: decide on a single approach: why do we need all templates in that attribute (why include obsolete ones), and then decide whether to use a property or a method
<adiroiban> danilos: i guess that we should try to see if a direct export of template will work... otherwise we should add a getTemplateStats method
<danilos> adiroiban, well, what we'd probably want is "getStatsForTemplates" instead (to get a bunch of them together)
<danilos> adiroiban, if you are planning to collate those, use it for reporting and such
<adiroiban> danilos: if we don't export all templates, then we will have to add a new method getAllTemplates
<danilos> adiroiban, there is already such a method
<adiroiban> danilos: and will add one more call into the API...
<danilos> adiroiban, it's called getTranslationTemplates
<adiroiban> danilos: so exporting all templates at once should make the API simpler
<danilos> adiroiban, if that's what we are going to be accessing templates for
<danilos> adiroiban, so, what are the use cases? if most of the time you'll be looping over current templates (you don't care about disabled templates for statistics, do you?), it'll make everybody else using the API write more code to ignore obsolete templates
<danilos> adiroiban, that sounds like a bad API decision to me, even if it keeps our API "simpler" (and soon enough people will ask for "getCurrentTemplates" to be exported as well, so it won't really be simpler :)
<adiroiban> danilos: yes. we can have an API exporting simple things, or and API that is simple to use :)
<adiroiban> danilos: the use case for getCurrentTemplates is not to report statistics, but rather to provide an API for reading and managing POTemplates
<adiroiban> danilos: like what template is linked to a gettext domain
<danilos> adiroiban, I'd say "full templates list" is for managing templates (that's where you want to get obsolete ones as well)
<adiroiban> danilos: or should allow UTC to batch process changes in the templates (ie. disable 20 templates in the last 2 series)
<adiroiban> danilos: for getting translation statistics I have opened bug 583979 . And those data will be exported using methods
<danilos> adiroiban, right, those are the methods that really rang a "bell" in my head: we are approaching all this in a totally unnatural order
<danilos> adiroiban, i.e. we should not be needing any new methods at all
<danilos> adiroiban, we should sanitize our interfaces a bit, sure, but this stuff is already possible and we should see what we need to do to export it
<adiroiban> danilos: it does not need to be new method, only new exports for existing methods
<danilos> adiroiban, sure, but we'd have to sanitize those interfaces first
<danilos> adiroiban, the names they have today doesn't help them :)
<adiroiban> danilos: ok, but getting back to the current IPOTemplate API branch.
<adiroiban> danilos: should we stop landing it and go work on sanitizing the interfaces?
<danilos> adiroiban, no, we just need to go over it a bit more carefully and make appropriate decisions for each of the parameters
<danilos> adiroiban, I'll be sending you an email later today about what I propose we change some more, and then you can either complain or agree and then we can land it
<adiroiban> danilos: ok :)
<danilos> adiroiban, this particular branch shouldn't be a hostage to nice-to-do interface changes that we won't do this week
<danilos> adiroiban, also, we should probably wait to land it after the release so there's plenty of time to QA it on edge and fix anything we need fixed
<adiroiban> danilos: ok
<bigjools> noodles775: want to swap reviews?
<noodles775> bigjools: sure
<bigjools> unless you're about to give me a monster :)
<noodles775> bigjools: nah, it's just 500lines of more test fixes after an ec2 run of the build gen. branch.
<bigjools> noodles775: talking of reviews, my +activereviews page tells me you've got a branch ready to land from a while ago
<bigjools> lp:~michael.nelson/lp-production-configs/dogfood-config-changes
<noodles775> bigjools: Actually, I thought that the tom et al landed al lproduction config changes?
<noodles775> bigjools: hence the 'community' next to your review.
<bigjools> noodles775: the branch scanner should have picked it up as merged I thought though? hmmm
<noodles775> bigjools: I don't think it has landed... I just meant that I thought tom lands those (at least he has in the past).
<bigjools> noodles775: yep, he does those
<noodles775> bigjools: nice... new unit tests :)
<bigjools> \o/
<noodles775> bigjools: why do you remove the proxy on the test case's instance var, rather than just when you need to set something that you normally wouldn't be able to?
<noodles775> bigjools: also, you could use FakeMethod there if you wanted to... it would mean that you don't need to set slave_called on the builder, which would also mean you don't need removeSecurityProxy right?
<bigjools> noodles775: proxy is removed because...ummm I can't remember, let me try without it again since I've changed the code a bit since I added it (for good reason at the time)
<noodles775> I'm guessing it's because you're setting builder.slave_called, which isn't a interface attribute (well, it's not an attribute at all except for your test is it?)
<bigjools> indeed
<bigjools> ah, it's because I can't override slaveStatusSentence otherwise
<bigjools> well, I suppose I could name it the same
<noodles775> Could you do the following in setUp: removeSecurityProxy(self.builder).slaveStatusSentence = FakeMethod(...)
<noodles775> Then your test could just check self.builder.slaveStatusSentence.call_count == 0
<bigjools> yeah, let me try it, I wasn't aware of FakeMethod
<noodles775> OK, with that r=me
<bigjools> noodles775: can you make FakeMethod return what you need?
<bigjools> result= by the looks of it?
<noodles775> bigjools: yup.
<bigjools> niiiice
<bigjools> noodles775: ok that all works but I still need the rSP :)
<bigjools> thanks for the review
<noodles775> bigjools: yeah, I just meant moving it so it's only used to set the one property, rather than storing the un-proxied version. NP.
<bigjools> Since the test doesn't need to care about security, I figured it would be better to unproxy it for everything
<bigjools> noodles775: line 42 of the diff: + self.obj.source_package_release.sourcepackagename,
<bigjools> one of my fears realised about inconsistent _ usage :(
<noodles775> bigjools: yes, I've only updated self.obj - the BinaryPackageBuild... I've not renamed other items (in this case SPR)... and don't think we should as part of this branch?
<bigjools> noodles775: that's fine, as long as you're going to fix it soon I guess
<noodles775> yep.
<bigjools> It's gonna take some effort to undo the muscle memory for typing Build and buildstate :)
<noodles775> Yeah.
<rockstar> Can I get a quick review from someone for a small branch?
<rockstar> abentley, hi, I was just about to start hunting for someone to do a review for me.
<rockstar> abentley, could I get you to review https://code.edge.launchpad.net/~rockstar/launchpad/no-duplicate-recipe-names/+merge/26080 ?
<abentley> rockstar, sure.
<rockstar> abentley, ta.
* noodles775 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
* gmb changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [gmb(http://is.gd/cpL9M)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [gmb(http://is.gd/cqgOz)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> rockstar, mumble?
<rockstar> abentley, sure, lemme see Mo off to school real quick.  5 mins tops.
<abentley> rockstar, okay.
<gary_poster> mars: ping?  Would like your review of one line change I mentioned yesterday: https://code.launchpad.net/~gary/launchpad/cookielib/+merge/26092
<gary_poster> who to bother...who to bother...
<gary_poster> sinzui: would you give me a review of http://cowboy00242.blogspot.com/2008/01/trivia-labs-dawn-breaks-over-marblehead.html ?  I think it should be extremely quick (it's an import fix to utilities/paste)
<gary_poster> lol
<gary_poster> or maybe https://code.launchpad.net/~gary/launchpad/cookielib/+merge/26092
<sinzui> gary_poster, I can
<sinzui> gary_poster, which do you want me to review first? The proposal or the article?
<gary_poster> sinzui: the propsal first please :-)
<sinzui> r=me
<gary_poster> thank you sinzui
<sinzui> +1 on the article
<gary_poster> lol
#launchpad-reviews 2010-05-27
<stub> https://code.edge.launchpad.net/~stub/launchpad/read-only-launchpad/+merge/26033
<rockstar> stub, can I get you to do a review for me?
<stub> yo
<rockstar> stub, https://code.edge.launchpad.net/~rockstar/launchpad/sourcepackagerecipe-name-contstraint/+merge/26122
<stub> rockstar: Is using a propertly like that how we do this elsewhere?
<stub> rockstar: Normally, we don't do this sort of check except in the UI code.
<stub> rockstar: And the database exception gets raised if something is broken and lets through invalid data
<rockstar> stub, well, I did the pre-imp with abentley, and that was his suggested method, which I liked.
<stub> You are issuing a database query every time name or owner is set, despite the fact nothing should ever get that far with values that cause a problem.
<stub> That is a lot of database queries to protect against input that should never arrive, and will be caught at the database layer.
<stub> I can see the point for an emergency cherry pick to edge, but I don't like it for long term
<rockstar> stub, so you're saying this should be implemented in the browser code only?
<stub> Yes, like all our other unique inputs.
<rockstar> stub, okay.  I'll look into that in the morning then.
<rockstar> stub, thanks for the review.  Please disapprove it.
<noodles775> Hi stub, I've just added a comment on your MP - I can't see anywhere where was_read_only is set initially?
<noodles775> Also, do you have a metric for dogfood minutes/production minutes? (see my last comment at: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-changes-build-generalisation-new/+merge/25594 )
* noodles775 changed the topic of #launchpad-reviews to: On Call: noodles775 || reviewing: stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> noodles775: It isn't set initially - that is why I use getattr(locals, 'was_read_only', None) instead of locals.was_read_only. There is nowhere sane to initialize it because there is no per-thread initialization
<noodles775> stub: yes, I understood the getattr, but afaics, it is *never* set?
 * noodles775 looks again
<stub> It is set at the end of that if statement
<stub>             self.thread_locals.was_read_only = is_read_only()
<noodles775> stub: yes, but that if statement is only executed if was_read_only is not None?
<stub> point.
 * stub wonders how his tests work at all
<noodles775> Ah, there were tests? Can you update the MP with the test command... I only requested another foundations person because I assumed there were no tests for this functionality.
<stub> There are tests. I'm fixing the last one of them now ;)
<noodles775> Great :)
<stub> noodles775: All pushed. There are other readonly mode tests too, but they are for bits I didn't touch
<stub> (trying to keep this minimal since we don't have much time to test, and I'm lazy)
<noodles775> stub: great, I'll take another look. Also, did you see my question about dogfood vs. production minutes above?
<stub> Dogfood isn't great for timings. Staging timings are better.  If we think it is too slow, or we can't wait to land on staging to be sure, I'll need to refactor the migration.
<stub> We don't really have a metric for dogfood. Staging we tend to double the times (faster machines, but three replicas to apply to)
<noodles775> stub: OK. Unfortunately we can't land it on staging just to find out (as it needs to land together with over 9k of code changes).
<stub> I can get a timing... I'll kick it off now so we will know.
<noodles775> Ah, great!
<noodles775> stub: you've got some staging database you can trash afterwards have you? or are you just not committing the transaction?
<stub> I'll skip the commit
<stub> Might make staging whOOPS a bit, but whatever.
<stub> noodles775: Is this landing this cycle?
<noodles775> We were hoping to... we've got it on dogfood at the moment, but haven't yet managed to push through a few hundred packages. At the moment its not looking likely, unless we get lots of packages through df today.
<noodles775> stub: it's expected that test_publication passed both with and without your changes right?
<stub> Yes. I'm fixing a race condition that is common under load but that we have no tests for.
<noodles775> OK, r=me
* jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer_ changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: stub, deryck || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> noodles775: It took 17 minutes on staging, so maybe 35-40 mins to apply to production.
<noodles775> stub: thanks. I assume that's way too long? What is acceptable?
<stub> Thats up to the release manager and losas
<noodles775> OK.
<stub> noodles775: This version should be identical and takes 8 minutes, so 16-20 mins on production: http://paste.ubuntu.com/440285/
<wgrant> noodles775: When you've a moment, can you please re-EC2 lp:~wgrant/launchpad/diffs-in-queue? I've fixed the failures from a few days ago.
<noodles775> Thanks stub... I'll update and send it off to ec2... but at this time I don't think I'll be landing it.. there seem to be other issues with teh current db-devel on dogfood that need looking in to.
<noodles775> wgrant: sure.
<wgrant> noodles775: Thanks.
* jelmer changed the topic of #launchpad-reviews to: On Call: noodles775, jelmer || reviewing: stub, stevenk || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: stevenk || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> win 34
<wgrant> Argh.
 * noodles775 is glad it's not just him who does that ;)
<bigjools> why don't you leave the 80s behind and get a graphical IRC client ;)
<noodles775> heh, you use vim too right? ;)
<maxb> bigjools: Find me a graphical irc client I can run in screen :-)
<maxb> Actually, I do run a graphical IRC client, I just connect it to my irssi in proxy mode :-)
<jelmer_> hi deryck
<deryck> Hi jelmer
<deryck> jelmer, thanks for the review!
<deryck> jelmer, so I used transaction.commit because that used in other tests.  I feel dumb not knowing, but is Store.of().flush() better to use here?
<bigjools> ah, the right tool for the job I say :)
<deryck> bigjools, is that in reply to my question?
<bigjools> deryck: ah no, I was replying to others before you
<bigjools> deryck: but FWIW flush() is always preferred to commit() if it works!
<bigjools> commit() makes tests *really* slow
<wgrant> Unless you're like bigjools and have a ridiculous SSD?
<bigjools> s/ridiculous/absolutely gorgeous/
<deryck> bigjools, right.  And I want to understand too -- flush may or may not commit, or it never commits?  i.e. flush just writes from memory to disk without commit?
<bigjools> correct
<bigjools> well, flushes from the cache
<deryck> gotcha.  I think I need commit in this case then, but will test and confirm.
<wgrant> flush() causes Storm to execute pending SQL.
<wgrant> You should rarely need to do that manually.
<deryck> Right
<wgrant> It probably (well, hopefully) won't hit disk.
<deryck> ah, right.  I see the distinction, thanks.  My mind is a bit fuzzy today.
* StevenK changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: stevenk || queue: [StevenK*3] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer_ changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: stevenk || queue: [StevenK*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<StevenK> jelmer: I've pushed a fix for the -config issue
<jelmer_> StevenK: cool - I've already approved that one, just waiting for Michael to review
 * jelmer_ wished Launchpad allowed Tweak votes like BundleBuggy used to
<noodles775> StevenK, jelmer_ : I just approved - with a note about lp_production_configs.
<wgrant> noodles775: Thanks -- both landed.
<noodles775> wgrant: np.
* sinzui changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: stevenk || queue: [StevenK*2, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer_ changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: sinzui || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> hi Curtis
<jelmer_> thanks for the non-Soyuz branches :-)
<noodles775> sinzui: when is PQM closing? I remember seeing an announcement, but can't find the email.
<sinzui> noodles775, I am confirming that now for an email. I asked for 22:00 UTC
<noodles775> sinzui: on which day?
<sinzui> today
<noodles775> sinzui: I've a branch that includes a schema patch that, according to stubs testing (when he approved it) will take 16-20mins on production. Are you ok with that?
<sinzui> ouch. yes I am
<noodles775> Thanks.
<sinzui> noodles775, StevenK. If you are concerned about not meeting the PQM deadline in 8 hours, I can look at your branches before they go to ec2 and provide a pre-emptive RC so that you are not penalised by ec2 variability
<noodles775> Thanks sinzui.
<StevenK> sinzui: I have a branch that just hit ec2
<StevenK> But I so don't want to stay up and babysit it
<sinzui> okay, I think that will be fine
<noodles775> jelmer_: are there other examples of wrapping a method for api exposure that prefix with an _?
<noodles775> (in ref. to your checkUpload branch).
<sinzui> noodles775, I think that means you want to rename the method to be public now
<sinzui> noodles775, I will refrain from suggesting a uses the rename hack since exposing a method explicitly makes it public
<jelmer_> noodles775: Maybe, I haven't actually checked...
<jelmer_> noodles775: I'll have a look now.
<noodles775> sinzui: in this case it seems it's just to use string args instead of the normal IFaces... it seems strange to prefix with _ imo. (sinzui: https://code.edge.launchpad.net/~jelmer/launchpad/can-upload-webcall/+merge/26141  for reference :) ).
<noodles775> jelmer_: also, why are you not returning True/False? It also seems strange to not return *anything* to mean yes, and raise an exception otherwise?
<jelmer_> noodles775: Originally I had it return the exception string and None when uploading was allowed.
<jelmer_> noodles775: I guess it really depends on whether we would want to expose the reason why the upload would not be possible to the user
<jelmer_> noodles775: there are different exception classes so in theory you could have different behaviour based on that.
<noodles775> jelmer_: we should never be expecting a 500, afaik (if you do raise an exception, it needs to use webservice_error(xxx) as in the other examples in interfaces/archive.py)
<jelmer_> noodles775: ok
<noodles775> jelmer_: in fact, there's already an InsufficientUploadRights exception defined there - it might be what you're after.
<noodles775> jelmer_: but I'm still unsure why you're not returning True/False.
 * noodles775 checks for other examples.
<jelmer_> noodles775: With True/False the user doesn't get an idea why the upload wouldn't be allowed (user not in packageset, pocket is release and distroseries is closed, etc)
<jelmer_> noodles775: The feature request doesn't require that though, so I'm happy to change it to a boolean.
<noodles775> jelmer_: in which case, would either returning True, or raising a descriptive webservice_error be better?
<jelmer_> noodles775: I would expect a descriptive webservice error to be better because it gives the user more information that they can use or ignore.
<noodles775> jelmer_: great. So if they can upload, return True, otherwise raise a descriptive webservice error.
<jelmer_> noodles775: ok
* jelmer_ changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> hi jelmer!  Can I chuck one on your queue?
<jelmer_> jtv: Hee Jeroen! Yeah, go ahead.
<jtv> dankuwel :)
* jtv changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: sinzui || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> noodles775: thanks for the review
<noodles775> jelmer_: np!
<sinzui> gary_poster, ping
<noodles775> sinzui: I have to run, so assuming you're keen to land your cache branch that jelmer reviewed, can you request a review from the next OCR?
<gary_poster> sinzui: hi
<sinzui> gary_poster, unping, noodles775 just offered
<gary_poster> ok
<sinzui> or I am confused
<noodles775> sinzui: ? I was saying I *can't* review it now...
<noodles775> :)
<sinzui> gary_poster, can you review https://code.launchpad.net/~sinzui/launchpad/cache-tweaks-0/+merge/26167
<gary_poster> sinzui, will be happy to, but may or may not have IS call now.  I'll start looking, and will inform you if I'm dragged away
<sinzui> ^ gary_poster your tales insight would be appreciated since I had to enter macro territory to make cache times variable
<gary_poster> cool.  my tales insight is questionable, but will give it a whirl
<sinzui> I am somewhat concerned that I may be the leading tales/metal developer on the Launchpad team. I blame attrition.
<gary_poster> LOL " Pre-implementation: no one, I really am guessing, really"
<stub> https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/26017 <- I need to land the extra logging at a minimum
<stub> jtv: ^^
<jtv> stub: it'll cost you a beer
<jtv> Oh, it's the one I looked at before
<stub> You only get Leo for this one ;)
<jtv> stub: grr...  just make sure there's a bug for the missing tests.
<jtv> Leo < Lao
<stub> I can revert the changes for the report - it needs to be rewritten anyway
<jtv> which are those?
<stub> Although that means it will certainly be broken, rather than untested.
<stub> pageperformancereport.py
<stub> The important bit to land is publication.py
<jtv> Since this is ongoing work I'd rather see it land as-is, with a bug filed for missing tests.
<stub> Bug #586421
<mup> Bug #586421: page-performance-report.py is untested <Launchpad Foundations:New> <https://launchpad.net/bugs/586421>
<jtv> stub: I've updated the MP
<jtv> jelmer: I'm off before the military seals off the city.  Hope you find all well with my branch!
<jtv> stub: still curfew tonight, right?
<stub> think so - midnight
<jtv> :(
<jelmer> jtv: Yeah, I think so
<jelmer> jtv: Have safe and fun evening
<jelmer> *a
<jtv> Thank you.  :-)
<gary_poster> sinzui: approved, with a few thouhts
<gary_poster> thoughts
<sinzui> thank, I am sure thoughts are needed
<gary_poster> :-)
* jelmer_ changed the topic of #launchpad-reviews to: On Call: jelmer || reviewing: || queue: [jtv, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jelmer_> sinzui: sorry, I've had to put your branch back in the queue
<jelmer_> Hopefully rockstar should be able to review it.
<sinzui> thanks. I do not intend to land it for 10.05 so I was not concerned.
<jelmer_> ah, great.
<rockstar> sinzui, I probably will not be taking my OCR day today.  I have things that need to get into 10.05
* jelmer_ changed the topic of #launchpad-reviews to: On Call: || reviewing: || queue: [jtv, sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> rockstar, understood. ping me if you think you need a pre-emptive rc
<jelmer_> sinzui: I need one actually..
<jelmer_> sinzui: Michael has asked me to fix some things in https://code.edge.launchpad.net/~jelmer/launchpad/can-upload-webcall/+merge/26141 that have now been adressed; it'd be nice if this change could make it into 10.05.
<jelmer_> sinzui: Would you perhaps be able to re-review that branch?
<sinzui> okay
<sinzui> jelmerI really do not understand why we have a public private method on the interface. It we expect other objects/callsites to know about it, it should not have a leading underscore.
<sinzui> jelmer why do we *need* a leading underscore?
<jelmer_> sinzui: there already is a checkUpload method that takes real objects and returns exceptions
<jelmer_> sinzui: the _checkUpload method is a wrapper around that with semantics better suitable for the web service
<sinzui> So this export also blocks the other method from being exported by accident
<jelmer_> yeah
<sinzui> jelmer r=me, rc=me
<jelmer_> sinzui: thanks!
<rockstar> sinzui, when is PQM closing?
<sinzui> 4.5 hours
<rockstar> sinzui, oh shit...
<rockstar> sinzui, I'll probably need a few RCs.
<rockstar> sinzui, why so early?
<sinzui> you can ask for an RC *now* better ask early than wait 10 hours
<sinzui> US and UK are off on monday
<rockstar> sinzui, oh crap...  I forgot.
<rockstar> sinzui, is it safe to assume you might be around on the weekend
<sinzui> I certainly will
<sinzui> I will send out an email about contacting me after I finish testing my RC: hack to send sms messages to my phone
<rockstar> sinzui, can you look at this really quick. Just needs code review.
<sinzui> sure
<sinzui> rockstar, am I correct in seeing you have been dupped into landing bug data changes?
<sinzui> rockstar you have my RC for the ec2 landing.
<sinzui> rockstar, I agree with BjornT regarding the test in https://code.edge.launchpad.net/~rockstar/launchpad/no-duplicate-recipe-names/+merge/26080
<rockstar> sinzui, do you have a suggestion?  BjornT is apparently not around.
<sinzui> I am going to look
<rockstar> I couldn't see another way around that.
<rockstar> sinzui, I'm happy to say it's a temporary test.
<rockstar> sinzui, basically, the patch needs to get in.
<sinzui> rockstar, we have 100's of UNIQUE constraints, and non are tested. drop the test
<sinzui> I landed a constraint a few months ago to force integrity error so that I could find bad code, but I did not test the unique constraint
<rockstar> sinzui, okay.
<rockstar> sinzui, test is gone.
<sinzui> faboo
<rockstar> sinzui, were you going to do the code-review for https://code.edge.launchpad.net/~rockstar/launchpad/no-recipe-dupes/+merge/26212 ?
<sinzui> rockstar, can you really do model work in the UI without making the import fascist angry
<rockstar> sinzui, I didn't notice any errors.
<sinzui> rockstar, this implementation is not safe for API.
<sinzui> I expect a set object to have a lookup method that API and browser can use
<rockstar> sinzui, yes, I proposed a fix in the model, and stub disapproved it.
<sinzui> what was his reason?
<rockstar> sinzui, because it was requiring a select query on every change.
<sinzui> There are no uses of store.find under browser/ this implementation gives the view more knowledge of the schema than the model has
<rockstar> sinzui, so, the plan is to get the db patch in, fix the UI, and then go about fixing the mutators for the API.
<rockstar> sinzui, so, how about if I move the store.find to be a staticmethod of the SourcePackageRecipe model?
<rockstar> And then I just call it from the browser code.
<rockstar> sinzui, I figured I'd have to do that anyway.
<sinzui> move your store core to the Set or utility object. Views lookup duplicate instances all the time
<rockstar> sinzui, okay.
<sinzui> rockstar, I am think about recent integrity issues in Packaging. Packaging does not have a Set. It has a utility object that lets us check for existence and we had to use that in several places to avoid insane instances
<rockstar> sinzui, okay.
<sinzui> ISourcePackageRecipeSource.exists() ?
<rockstar> sinzui, yeah, that's probably a better plan.
 * rockstar goes to work.
<rockstar> sinzui, fixed: https://code.edge.launchpad.net/~rockstar/launchpad/no-recipe-dupes/+merge/26212
 * sinzui looks
<sinzui> rockstar, this is what I expected :) I also expected a doc test to demonstrate how it is used. there is no doc test for recipes though
<sinzui> rockstar, what do I read to learn how to use the api?
<rockstar> sinzui, nothing.  We need to shift where our tests are, and make documentation.
<rockstar> Right now, there are unit tests for the API, but not docs.
<sinzui> TestSourcePackageRecipe ?
<sinzui> I see the creation test there
<sinzui> rockstar, I know you do not want to read this. I think we need a short test much like setup in the view test that directly verifies exists()
<rockstar> sinzui, okay. I can do that in a unit test.  It's not a big deal.  I have to get an RC for it now anyway.
<sinzui> yes, and I will give it to you for all your troubles
<rockstar> sinzui, did you see my other two branches with a rc request?
<sinzui> I think I gave them to you.
 * sinzui looks again
<rockstar> sinzui, test added, pushing now.
<sinzui> I was negligent. I see I did not do as I said I would
<rockstar> sinzui, I don't think negligent is the right word there.  :)  Busy, scatterbrained, harassed, not negligent.
<sinzui> If I do a bad job here, no one will ask me to be the release manager again@
<sinzui> !
<sinzui> I see my SMS forwarder needs some python 2.6 love
<sinzui> rockstar, I think we are done. three rc and the reviews are approved
<rockstar> sinzui, awesome, thanks.
<rockstar> sinzui, is PQM closed now then?
<sinzui> It closes in 90 minutes
<thumper> sinzui: are you RM this time?
<sinzui> I am.
<sinzui> As I said, If I cock this up, I will not be asked to do this again :)
<thumper> sinzui: can I chat briefly about a couple of branche?
<sinzui> yes
<thumper> sinzui: skype?
<sinzui> I am ready
#launchpad-reviews 2010-05-28
<jtv> good morning adeuring!  Any chance I could bother you for a review that didn't make it through the queue last night?  https://code.launchpad.net/~jtv/launchpad/bug-409686/+merge/26179
<adeuring> jtv: sure
<jtv> adeuring: wonderful, thanks!
* adeuring changed the topic of #launchpad-reviews to: On Call: adeuring || reviewing: jtv || queue: [sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> jtv: r=me
<jtv> adeuring: thanks!
<jtv> adeuring: good point on the LOGIN... how about USERNAME?
<adeuring> jtv: right, that looks much nicer!
<jtv> Oh
<jtv> idea
<jtv> No wait, no idea.
<jtv> Capital letters are the only good way I can think of there to say "this isn't literal"
<jtv> I thought of using quotes, but that may be confusing because the instructions do contain a quoted string.  So the quotes would have different meanings.
<adeuring> right. BTW, how do we handle these "placeholders" elsewhere?
<jtv> Well ideally, it'd be nice if I could fill in the real username there.  But the message may be going out to teams.
* adeuring changed the topic of #launchpad-reviews to:  On Call: adeuring || reviewing: - || queue: [sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> jtv: We know the LP user when we generate the email, don't we? So, can't we simply insert his login name?
<jtv> Hmm...  I'd have to iterate over the team if there was one, I guess.
<jtv> How about I file a bug for it as a separate job?
<adeuring> jtv: Ah, OK. So, let's leave it as it is
<jtv> hey danilos
<danilos> jtv, hey
* StevenK changed the topic of #launchpad-reviews to:  On Call: adeuring || reviewing: - || queue: [sinzui, jelmer,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On Call: adeuring || reviewing: - || queue: [sinzui, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> StevenK: I can review your branch if you show me where I can find it
<StevenK> adeuring: Sure -- https://code.edge.launchpad.net/~stevenk/launchpad/poppy-sftp-updates/+merge/26279
<StevenK> Wow. Was it that bad ...
* adeuring changed the topic of #launchpad-reviews to: On Call: adeuring || reviewing: StevenK || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> StevenK: your branch screams for a few tests ;) For example, it is great that you added the missing "import FileIsADirectory" but the fact that this was not spotted earlier is a clear indication that SFTPFile.writeChunk(9 is not properly tested.
<StevenK> adeuring: Right, I'm writing a test for that now.
<adeuring> StevenK: great, thanks!
<adeuring> StevenK: similary, would you mind to add tests for the constructor of class Hooks?
<adeuring> StevenK: well, actually, you are testing that Hooks() accepts the prefix parameter, but could you add a test thats ensures that it is used in the filename?
 * adeuring needs a better mental OCR algorithm
<adeuring> I misread "twisted" for 2test"....
<StevenK> adeuring: If you're still here, I'll be pushing the test fixes to my branch in a few minutes.
<adeuring> StevenK: sure, go ahaed
<StevenK> adeuring: Pushed.
 * adeuring is looking
<adeuring> StevenK: thanks for the new tests! r=me
<StevenK> adeuring: Thanks for the review :-)
<sinzui> StevenK, what are the consequences of landing after the release?
<StevenK> sinzui: You'd prefer to have this land as a CP?
<sinzui> I just want to understand the urgency.
<StevenK> sinzui: It has a feature that Julian thinks is fairly important, but if you'd rather it was CP'd, that's okay too.
<sinzui> okay, you have my RC on contingent you update the XXX comment
<StevenK> Which one?
<sinzui> 51 # XXX: Eww bug=586695
<StevenK> Ah. You'd prefer something more descriptive? :-)
<StevenK> sinzui: Done, and pushed.
<sinzui> thanks
<StevenK> sinzui: Fixed harder, based on your XXX style comment. Anyway, throwing it through ec2
<sinzui> well. I have no confidence in ec2 in the moment. I think your changes are isolated so your own testing gives me more confidence.
<StevenK> Poor ec2. What did it ever do to deserve such .... Oh. Never mind. :-P
<StevenK> sinzui: I'm happy to pqm-submit, but I have not run the entire test suite over the code.
<sinzui> lets use ec2
<StevenK> sinzui: Mind you, I share your vote of no confidence.
<adeuring> sinzui: i tried this morning to find an MP from you but couldn't find any
<sinzui> adeuring, there was confusion yesterday and my name appears to have been added back. I have no branches thanks.
* sinzui changed the topic of #launchpad-reviews to: On Call: adeuring || reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> sinzui: ah, ok
* StevenK changed the topic of #launchpad-reviews to: On Call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring 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
#launchpad-reviews 2010-05-29
<stub> sinzui: If you are still there, https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/26017 is a branch that got eaten by ec2 (or something) before PQM closed. I want to get the logging change in so I have real logs to play with next cycle.
#launchpad-reviews 2011-05-23
<sepidev> hi guys, i'm having trouble with bzr, whenever I want to push something to launchpad or pull or branch something I get this error
<sepidev> bzr: ERROR: Connection error: curl connection error (couldn't connect to host)
<sepidev> on https://code.launchpad.net/~sikon/steadyflow/trunk/.bzr/smart
<sepidev>  HELP: bzr: ERROR: Connection error: curl connection error (couldn't connect to host)
<StevenK> This is the wrong channel for that question, I'd suggest you ask in #launchpad or #bzr
