| gary_poster | mwhudson: ping? | 01:02 |
|---|---|---|
| mwhudson | gary_poster: hi | 01:02 |
| 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. | 01:05 |
| mwhudson | gary_poster: maybe a little bit | 01:05 |
| gary_poster | mwhudson: :-) | 01:05 |
| mwhudson | (also, i'm currently using APUE as a monitor stand :) | 01:06 |
| * mwhudson looks at the branch | 01:06 | |
| gary_poster | LOL | 01:06 |
| 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. | 01:07 |
| === Ursinha is now known as Ursinha-afk | ||
| mwhudson | gary_poster: i reviewed the branch | 02:27 |
| 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. | 02:56 |
| mars | mwhudson, thanks for the review | 02:56 |
| mwhudson | mars: i use two books, the other one is "xUnit test patterns" | 02:57 |
| mars | that's on my to-read list | 02:58 |
| mars | reading Release It! right now | 02:58 |
| 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 :) | 02:58 |
| mwhudson | (as i did for /most/ of my books) | 02:58 |
| mars | mwhudson, have a sec for a question about that review? | 02:59 |
| mars | just want to be clear on the process group leader problem you mentioned | 03:00 |
| mwhudson | mars: sure | 03:01 |
| mars | mwhudson, ok, so you are worried about line 58, "assert original_process_group != os.getpid()", right? | 03:02 |
| mwhudson | mars: yes | 03:02 |
| mars | ok | 03:02 |
| 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 | 03:02 |
| mars | mwhudson, so I don't understand the usePTY=True bit. What is that setting, and what would it do? | 03:03 |
| mwhudson | mars: it's a buildbot thing | 03:03 |
| mwhudson | if you use it buildbot runs your subprocess with a pseudo tty attached | 03:03 |
| mwhudson | and, thanks to a twisted api oddity, in a new session (in the setsid() sense of session) | 03:04 |
| mwhudson | which implies creating a process group | 03:04 |
| mars | wow, that's arcane | 03:04 |
| mwhudson | yeah, it is a bit | 03:04 |
| mars | how did you find that out? | 03:04 |
| mwhudson | 03:04 | |
| mwhudson | specifically http://permalink.gmane.org/gmane.comp.python.buildbot.devel/5680 | 03:05 |
| mars | I think I meant "what prompted you to discover this particular arcane bit of knowledge" :) | 03:05 |
| mwhudson | mars: review paranoia ... "i wonder if buildbot starts processes in their own process group" | 03:06 |
| 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" | 03:06 |
| mwhudson | mars: i thought there was a chance it would do it always | 03:07 |
| mars | well, guess what make -> test_on_merge.py -> bin/test were doing before! | 03:07 |
| mwhudson | oh right | 03:07 |
| mwhudson | yes | 03:07 |
| mwhudson | i think the shell use case is more compelling tbh | 03:07 |
| mars | sorry? | 03:07 |
| mwhudson | i think not being able to type ./test_on_merge.py in a shell would be bad | 03:08 |
| 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? | 03:08 |
| mars | _pythonpath is supposed to be a bridge [hack] | 03:09 |
| mwhudson | mars: well, ./bin/py test_on_merge.py fails too | 03:09 |
| mars | oh. Eww. | 03:09 |
| mwhudson | mars: the reexec could be done with bin/py i guess | 03:09 |
| mwhudson | i guess you could fork() and have the parent just waitpid on the child, that would have the same effect i guess | 03:10 |
| mars | mwhudson, so what would the re-exec do? | 03:10 |
| mars | yes, fork() would work nicely too | 03:11 |
| mwhudson | mars: get us into a situation where we're not the process group leader | 03:11 |
| mars | gives you the new PID you need for the process group swapping | 03:11 |
| mwhudson | perhaps fork would be cleaner | 03:12 |
| mwhudson | (man unix is crazy) | 03:12 |
| 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. | 03:15 |
| mars | at least, that has been my experience with writing application software | 03:16 |
| 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. | 03:18 |
| mwhudson | i don't know that there is much excuse for job control to have invaded the unix kernel as much as it did | 03:19 |
| mwhudson | mars: i think fork() would be preferable too, it hadn't occurred to me when i wrote my version | 03:20 |
| 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 :/ | 03:20 |
| mwhudson | mars: although, sys.executable will never be ./bin/py so that particular objection isn't actually valid | 03:21 |
| mwhudson | mars: i think that kind of thing is one of those horrible unportable unix things isn't it? | 03:21 |
| mwhudson | though these days just supporting linux and os x would be fine | 03:22 |
| mars | mwhudson, so you run it with bin/py, then it re-execs itself with sys.executable without your permission... that could be annoying. | 03:22 |
| mars | that is another reason to prefer fork() | 03:23 |
| mars | mwhudson, is there anything that has to be done when executing the fork() beyond... os.fork(), check if child? | 03:24 |
| mwhudson | mars: bin/py ends by execing with its sys.executable | 03:24 |
| mars | ah | 03:24 |
| mwhudson | but this is all silly, fork() is better | 03:24 |
| mwhudson | mars: no, i don't think so, pid = os.fork(); if pid != 0: os.waitpid(os.P_WAIT, pid) | 03:25 |
| mwhudson | i think? | 03:25 |
| mars | no messing with TTYs or streams or whatever? Just remembering the hassle to correctly fork a daemon... | 03:25 |
| mars | maybe File descriptors - daemonizing requires you to mess with file descriptors | 03:26 |
| mwhudson | i think because the parent process isn't doing anything we're ok | 03:27 |
| mwhudson | it would be worth checking that control-c still stops the test run | 03:27 |
| mwhudson | (but i think it will, that much worked with the spawnl version) | 03:28 |
| mars | hmm, that is something else I did not check | 03:28 |
| mars | how EINTR is handled in my script | 03:28 |
| 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 | 03:29 |
| mars | :D | 03:29 |
| mars | mwhudson, so should it fork always, just for simplicity's sake? | 03:31 |
| mars | I like simplicity here | 03:31 |
| mwhudson | yes that makes sense | 03:31 |
| mars | actually that makes is much simpler | 03:31 |
| mars | no more shenanigans then | 03:31 |
| mars | just os.fork(), os.setpgrp() | 03:32 |
| mars | oh, wait | 03:32 |
| mars | nope | 03:32 |
| mars | still need the shenanigans | 03:32 |
| 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 | 03:33 |
| mars | mwhudson, xvfb shares test_on_merge's process group, so os.killpg(9) .... | 03:33 |
| 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. | 03:37 |
| mars | mwhudson, thanks for the review. I'll write some code. | 03:37 |
| mwhudson | mars: i don't know _why_ control-c ing test_on_merge works in my branch, only that it does :) | 03:40 |
| mars | try using a test harness that ignores SIGTERM, then you might see the problem | 03:41 |
| mwhudson | mars: backtracking a bit, | 03:41 |
| mwhudson | <mars> just os.fork(), os.setpgrp() | 03:41 |
| mwhudson | is precisely not what we want: it's what the shell did | 03:41 |
| mwhudson | and we don | 03:41 |
| mwhudson | 't want to follow the fork with a setpgrp(), that's the point | 03:42 |
| 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? | 03:44 |
| mars | s/"child process's group"/xvfb-run prcoess group/ | 03:44 |
| 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 | 03:46 |
| mwhudson | i think? | 03:46 |
| mars | mwhudson, correct. | 03:47 |
| 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) | 03:53 |
| mwhudson | mars: right | 03:53 |
| thumper | mars, mwhudson: can one of you approve this one ? https://code.edge.launchpad.net/~thumper/launchpad/send-mail-job-logging/+merge/25939 | 04:05 |
| mars | thumper, doing PDR, maybe mwhudson can take it? | 04:05 |
| mars | it is already really late here | 04:06 |
| mwhudson | thumper: done | 04:09 |
| thumper | ta | 04:09 |
| stub | https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/26017 <- should be a quick one | 08:35 |
| 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 | 09:56 |
| stub | noodles775: Your testing on dogfood so we don't need contingency patches. | 10:00 |
| 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. | 10:02 |
| stub | Not really - once it is in production, the old data becomes out of date. | 10:02 |
| stub | And it won't be there anyway, as the table will be dropped by the upgrade procedure :) | 10:03 |
| 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). | 10:03 |
| === 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. :) | 10:33 |
| stub | k | 10:34 |
| 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. :-) | 10:40 |
| 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) | 10:42 |
| stub | k. I can extract it to a local function so I can still access the same scope. | 10:42 |
| jtv | Yes, require_args could be a bit painful. Frankly I don't see the value of making it local. | 10:43 |
| stub | I think the new version is less readable | 10:47 |
| 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.) | 10:48 |
| stub | That seems to work and is clearer anyway | 10:50 |
| stub | And the extension record parsing breaks out nicer too | 10:54 |
| stub | Oh poo... mixed it up with uncommitted changes | 10:58 |
| jtv | stub: sorry, distractions... back in a bit | 10:58 |
| 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. | 11:05 |
| jtv | The nice thing about "return" is that all flows of control come back together so unambiguously. | 11:06 |
| stub | Pushing new version now. I think this meets all your criteria | 11:06 |
| jtv | diff still updating... | 11:08 |
| stub | jtv: Done | 11:09 |
| jtv | stub: welcome back... looks like we both got cut off despite different providers. | 11:32 |
| jtv | stub: I was just asking about tests. | 11:32 |
| stub | There aren't any | 11:33 |
| jtv | Yes, that part was clear to me. :) | 11:33 |
| stub | Rationalization is that we don't know what the report looks like yet | 11:33 |
| 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? | 11:35 |
| 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. :-) | 11:36 |
| * jtv is over-using smileys to hide a mean mood. | 11:37 | |
| 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. | 11:38 |
| jtv | Cool. I'll make a note. | 11:38 |
| === 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 | ||
| === mrevell is now known as mrevell-lunch | ||
| === matsubara-afk is now known as matsubara | ||
| === salgado-afk is now known as salgado | ||
| danilos | adiroiban, btw, we'll have to change property names on bug 525371 at least | 13:24 |
| mup | Bug #525371: API for reading POTemplates attributes <api> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/525371> | 13:24 |
| === 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 ? :) | 13:25 |
| danilos | adiroiban, i.e. things like "all_potemplates" should be "translation_templates", and "all_pofiles" should be "translation_files" | 13:25 |
| danilos | adiroiban, "potemplate" name is a "gettextism" and we are gradually getting rid of those | 13:25 |
| adiroiban | danilos: ok. I was using the other properties like „all_milestones” | 13:25 |
| adiroiban | as guidelines for naming | 13:25 |
| danilos | adiroiban, not at all :) the problem is that I haven't had enough time to properly consider all this | 13:26 |
| adiroiban | ok | 13:26 |
| danilos | adiroiban, you are coding too fast :P | 13:26 |
| adiroiban | then „all_translation_templates” ? | 13:26 |
| danilos | adiroiban, no, not "all" | 13:26 |
| adiroiban | ok | 13:26 |
| 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 | 13:27 |
| 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 | 13:28 |
| adiroiban | danilos: ok. | 13:28 |
| adiroiban | danilos: then I will stop the work on the API and will wait further orders | 13:29 |
| 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 | 13:30 |
| adiroiban | danilos: since I am new to Rosetta developement and I new little about your future plans | 13:30 |
| adiroiban | it is hard for me to go allong and implement exactly what you want | 13:30 |
| danilos | adiroiban, not to worry :) | 13:30 |
| adiroiban | danilos: so what do you suggest I should do next ? (beside getting rid of gettext like names ) | 13:31 |
| 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"? | 13:31 |
| 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 | 13:32 |
| adiroiban | danilos: getTranslationsTemplates was the old interface attribute | 13:32 |
| adiroiban | danilos: but lazr.restful can not export a method as an attribute | 13:32 |
| danilos | adiroiban, right, no reason to not drop it, I guess? if it's not really used anywhere | 13:32 |
| adiroiban | danilos: this is why I had to add a new property to export that method | 13:32 |
| danilos | adiroiban, right, but that taints the interface unnecessarily: if it can be a property, it should be a property | 13:33 |
| danilos | adiroiban, if it can't be a property it should be a method (both exported and internal) | 13:33 |
| danilos | adiroiban, some reasons behind some of these interfaces are performance, and they are sometimes harder to grasp | 13:34 |
| adiroiban | danilos: ok. then I will have to file a bug for each such property | 13:34 |
| danilos | adiroiban, well, why do you believe this shouldn't be exported as a method? | 13:34 |
| adiroiban | danilos: for example IRosettaStats is full of messageCount(self, langauge=None) | 13:34 |
| danilos | adiroiban, IRosettaStats needs serious refactoring | 13:35 |
| danilos | adiroiban, it's totally, utter, crack | 13:35 |
| adiroiban | danilos: and in most cases the that method just returns the cached data from the table | 13:35 |
| danilos | adiroiban, which is exactly what we want most of the time | 13:35 |
| danilos | adiroiban, but, the problem with IRosettaStats is that it represents two different interfaces (language=None? wtf?) | 13:36 |
| danilos | adiroiban, the names of the properties are totally outdated on it as well | 13:36 |
| danilos | adiroiban, but anyway, I don't want to dwell on IRosettaStats in particular | 13:36 |
| danilos | adiroiban, it's an interface that needs to go out, basically | 13:36 |
| adiroiban | danilos: yes, IRosettaStats is a mess and that language=None is only implemened by IPOTemplate in a very strange way | 13:37 |
| danilos | adiroiban, that strange way is called "broken" way :) | 13:37 |
| adiroiban | danilos: the idea is that by exporting a data as an API attribute, we can get all attribute in one go | 13:37 |
| danilos | adiroiban, not sure what you mean? you can't do that with a method? | 13:38 |
| 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) | 13:38 |
| danilos | adiroiban, API should *always* return cached data | 13:38 |
| adiroiban | danilos: for API attributes, or for both data returned by attributes and methods | 13:39 |
| danilos | adiroiban, for anything that's expensive to calculate, if we have cached data, that's what we should return | 13:40 |
| 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 ? | 13:40 |
| 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 | 13:41 |
| adiroiban | danilos: ok :) back to IPOTemplate | 13:41 |
| danilos | adiroiban, right :) | 13:42 |
| 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 | 13:42 |
| 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” ? | 13:42 |
| 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 | 13:43 |
| === mrevell-lunch is now known as mrevell | ||
| danilos | adiroiban, sure, the same holds for the property, but there's no reason to introduce two things which basically do the same thing | 13:43 |
| adiroiban | danilos: you can change the parrent and call the same attribute | 13:44 |
| danilos | adiroiban, the important thing that I don't know an answer to is how does API support slicing | 13:44 |
| === 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 | 13:47 |
| adiroiban | danilos: if you want to export a subset, then a method is used | 13:47 |
| danilos | adiroiban, the examples on https://dev.launchpad.net/API/ImplementingAPIs for collections are pretty good | 13:47 |
| 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 | 13:48 |
| danilos | adiroiban, I think that "full" vs "subset" isn't, and shouldn't be a criterion to decide upon | 13:48 |
| danilos | adiroiban, it should be the sanity of API | 13:48 |
| 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 | 13:49 |
| 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 | 13:49 |
| danilos | adiroiban, one example is not conclusive, it's just an example | 13:50 |
| danilos | adiroiban, "adminmembers" on ITeam is a counter-example | 13:50 |
| adiroiban | danilos: true. | 13:52 |
| danilos | adiroiban, also, we should seriously consider not snapshooting such big exports | 13:52 |
| danilos | adiroiban, (I am worried about ~1500 templates on ubuntu distroseries being snapshotted) | 13:52 |
| adiroiban | danilos: so how you do you suggest we should export the templates attached to a series(entity) ? | 13:52 |
| danilos | adiroiban, I am not totally sure | 13:53 |
| danilos | adiroiban, properties are nicer because we don't have to add person parameter :) | 13:53 |
| 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 | 13:53 |
| 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 | 13:54 |
| 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) | 13:55 |
| adiroiban | ? | 13:55 |
| danilos | adiroiban, I don't know :) | 13:56 |
| danilos | adiroiban, that's what I am wondering as well | 13:56 |
| === Ursinha-afk is now known as Ursinha | ||
| adiroiban | danilos: ok. I could not find anything regarding „snapshot” on dev.lp.net, but I will look into the source code | 13:56 |
| adiroiban | danilos: we had the same problem with snapshot when we exported all languages from Launchpad | 13:57 |
| danilos | adiroiban, I am pretty sure what it does is instead return only links to objects which you have to load then | 13:57 |
| danilos | adiroiban, but, it's more interesting to know if it batches or not | 13:57 |
| 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 | 13:57 |
| adiroiban | s/do/so/ | 13:58 |
| 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 | 13:58 |
| 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 | 13:58 |
| 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 | 13:59 |
| danilos | adiroiban, but, we'd have to think about what's the best way out of it | 13:59 |
| 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 | 14:00 |
| 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 | 14:00 |
| danilos | adiroiban, well, what we'd probably want is "getStatsForTemplates" instead (to get a bunch of them together) | 14:01 |
| danilos | adiroiban, if you are planning to collate those, use it for reporting and such | 14:01 |
| adiroiban | danilos: if we don't export all templates, then we will have to add a new method getAllTemplates | 14:02 |
| danilos | adiroiban, there is already such a method | 14:02 |
| adiroiban | danilos: and will add one more call into the API... | 14:02 |
| danilos | adiroiban, it's called getTranslationTemplates | 14:02 |
| adiroiban | danilos: so exporting all templates at once should make the API simpler | 14:02 |
| danilos | adiroiban, if that's what we are going to be accessing templates for | 14:02 |
| 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 | 14:03 |
| 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 :) | 14:04 |
| adiroiban | danilos: yes. we can have an API exporting simple things, or and API that is simple to use :) | 14:08 |
| adiroiban | danilos: the use case for getCurrentTemplates is not to report statistics, but rather to provide an API for reading and managing POTemplates | 14:09 |
| adiroiban | danilos: like what template is linked to a gettext domain | 14:10 |
| danilos | adiroiban, I'd say "full templates list" is for managing templates (that's where you want to get obsolete ones as well) | 14:10 |
| adiroiban | danilos: or should allow UTC to batch process changes in the templates (ie. disable 20 templates in the last 2 series) | 14:11 |
| adiroiban | danilos: for getting translation statistics I have opened bug 583979 . And those data will be exported using methods | 14:12 |
| 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 | 14:14 |
| danilos | adiroiban, i.e. we should not be needing any new methods at all | 14:14 |
| 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 | 14:15 |
| adiroiban | danilos: it does not need to be new method, only new exports for existing methods | 14:16 |
| danilos | adiroiban, sure, but we'd have to sanitize those interfaces first | 14:16 |
| danilos | adiroiban, the names they have today doesn't help them :) | 14:17 |
| adiroiban | danilos: ok, but getting back to the current IPOTemplate API branch. | 14:17 |
| adiroiban | danilos: should we stop landing it and go work on sanitizing the interfaces? | 14:18 |
| danilos | adiroiban, no, we just need to go over it a bit more carefully and make appropriate decisions for each of the parameters | 14:19 |
| 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 | 14:19 |
| adiroiban | danilos: ok :) | 14:19 |
| danilos | adiroiban, this particular branch shouldn't be a hostage to nice-to-do interface changes that we won't do this week | 14:20 |
| 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 | 14:21 |
| adiroiban | danilos: ok | 14:21 |
| bigjools | noodles775: want to swap reviews? | 15:31 |
| noodles775 | bigjools: sure | 15:31 |
| bigjools | unless you're about to give me a monster :) | 15:32 |
| noodles775 | bigjools: nah, it's just 500lines of more test fixes after an ec2 run of the build gen. branch. | 15:32 |
| bigjools | noodles775: talking of reviews, my +activereviews page tells me you've got a branch ready to land from a while ago | 15:33 |
| bigjools | lp:~michael.nelson/lp-production-configs/dogfood-config-changes | 15:33 |
| noodles775 | bigjools: Actually, I thought that the tom et al landed al lproduction config changes? | 15:35 |
| noodles775 | bigjools: hence the 'community' next to your review. | 15:35 |
| bigjools | noodles775: the branch scanner should have picked it up as merged I thought though? hmmm | 15:36 |
| 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). | 15:38 |
| bigjools | noodles775: yep, he does those | 15:38 |
| noodles775 | bigjools: nice... new unit tests :) | 15:39 |
| bigjools | \o/ | 15:39 |
| 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? | 15:40 |
| 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? | 15:45 |
| 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) | 15:46 |
| 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?) | 15:47 |
| bigjools | indeed | 15:47 |
| bigjools | ah, it's because I can't override slaveStatusSentence otherwise | 15:47 |
| bigjools | well, I suppose I could name it the same | 15:48 |
| noodles775 | Could you do the following in setUp: removeSecurityProxy(self.builder).slaveStatusSentence = FakeMethod(...) | 15:49 |
| noodles775 | Then your test could just check self.builder.slaveStatusSentence.call_count == 0 | 15:50 |
| bigjools | yeah, let me try it, I wasn't aware of FakeMethod | 15:50 |
| noodles775 | OK, with that r=me | 15:51 |
| bigjools | noodles775: can you make FakeMethod return what you need? | 15:52 |
| bigjools | result= by the looks of it? | 15:52 |
| noodles775 | bigjools: yup. | 15:52 |
| bigjools | niiiice | 15:53 |
| bigjools | noodles775: ok that all works but I still need the rSP :) | 15:59 |
| bigjools | thanks for the review | 15:59 |
| 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. | 16:00 |
| bigjools | Since the test doesn't need to care about security, I figured it would be better to unproxy it for everything | 16:01 |
| === matsubara is now known as matsubara-lunch | ||
| bigjools | noodles775: line 42 of the diff: + self.obj.source_package_release.sourcepackagename, | 16:14 |
| bigjools | one of my fears realised about inconsistent _ usage :( | 16:15 |
| 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? | 16:15 |
| bigjools | noodles775: that's fine, as long as you're going to fix it soon I guess | 16:16 |
| noodles775 | yep. | 16:16 |
| bigjools | It's gonna take some effort to undo the muscle memory for typing Build and buildstate :) | 16:17 |
| noodles775 | Yeah. | 16:18 |
| === salgado is now known as salgado-lunch | ||
| === matsubara-lunch is now known as matsubara | ||
| === noodles785 is now known as noodles775 | ||
| === salgado-lunch is now known as salgado | ||
| rockstar | Can I get a quick review from someone for a small branch? | 18:44 |
| rockstar | abentley, hi, I was just about to start hunting for someone to do a review for me. | 18:54 |
| rockstar | abentley, could I get you to review https://code.edge.launchpad.net/~rockstar/launchpad/no-duplicate-recipe-names/+merge/26080 ? | 18:54 |
| abentley | rockstar, sure. | 18:55 |
| rockstar | abentley, ta. | 18:56 |
| === 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? | 19:11 |
| rockstar | abentley, sure, lemme see Mo off to school real quick. 5 mins tops. | 19:19 |
| abentley | rockstar, okay. | 19:19 |
| gary_poster | mars: ping? Would like your review of one line change I mentioned yesterday: https://code.launchpad.net/~gary/launchpad/cookielib/+merge/26092 | 20:25 |
| gary_poster | who to bother...who to bother... | 20:53 |
| 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) | 20:55 |
| gary_poster | lol | 20:55 |
| gary_poster | or maybe https://code.launchpad.net/~gary/launchpad/cookielib/+merge/26092 | 20:55 |
| sinzui | gary_poster, I can | 21:04 |
| sinzui | gary_poster, which do you want me to review first? The proposal or the article? | 21:04 |
| gary_poster | sinzui: the propsal first please :-) | 21:06 |
| sinzui | r=me | 21:06 |
| gary_poster | thank you sinzui | 21:08 |
| sinzui | +1 on the article | 21:08 |
| gary_poster | lol | 21:08 |
| === matsubara is now known as matsubara-afk | ||
| === salgado is now known as salgado-afk | ||
| === Ursinha is now known as Ursinha-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!