| === StevenK changed the topic of #launchpad-reviews to: On call: StevenK || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| thumper | StevenK: we can talk in public you know :) | 02:55 |
|---|---|---|
| StevenK | We could ... people might talk, then :-P | 02:57 |
| lifeless | or contribute | 03:03 |
| StevenK | thumper: I've addressed almost all of your concerns for db-add-ifp-job, I'm just having trouble writing InitialiseDistroSeriesJob.create() since DistributionJobDerived.create() has been binned, and could use some pointers. Or code. :-) | 03:10 |
| thumper | ok | 03:11 |
| thumper | what isn't working? | 03:11 |
| StevenK | thumper: I'm not at all certain what create() should be returning -- and to answer one of your questions on the MP, the code in .create() was checking jobs so that if a job for a specific distroseries was already in the queue, a second wouldn't be created | 03:12 |
| thumper | why would you be calling it twice? | 03:13 |
| thumper | I suppose it is bad to try twice | 03:13 |
| lifeless | race conditions | 03:13 |
| thumper | the problem with that is that there is go guarantee that even with checking that you won't get two | 03:13 |
| StevenK | Because InitialiseDistroSeries is one-shot. You take a empty distroseries and copy a lot of stuff | 03:14 |
| thumper | s/go/no/ | 03:14 |
| thumper | if you have two requests going to two different app servers at the same time | 03:14 |
| thumper | both will look and not see the job there | 03:14 |
| thumper | and both will create it | 03:14 |
| thumper | if you have to be sure, you should add a database constraint | 03:14 |
| thumper | with a unique partial index | 03:15 |
| StevenK | Okay, I'll keep that in mind | 03:15 |
| thumper | you want the create method to return the new distibutionjob | 03:17 |
| StevenK | thumper: Doing that causes other tests to fail since DistributionJob doesn't have a run() method, but InitialiseDistroSeriesJob does | 03:20 |
| thumper | StevenK: take a look at model/branchjob.py line 271 | 03:23 |
| StevenK | thumper: What should I RTFM for to add the database constraint? | 03:37 |
| thumper | StevenK: it is the create index syntax that has a where clause (I think) | 03:42 |
| lifeless | or you can just handle the job existing twice when it executes | 03:46 |
| thumper | lifeless: partial index would arguably be better | 03:50 |
| lifeless | thumper: why? | 03:50 |
| thumper | because it better represents the reality of the situation that there should be at most one | 03:51 |
| lifeless | so, a partial index means that a collision in the web ui will result in a collision error | 03:51 |
| lifeless | which is fugly | 03:51 |
| lifeless | and one that is caught outside the template code so can't be made to look good | 03:52 |
| thumper | ahm... | 03:52 |
| thumper | no | 03:52 |
| lifeless | secondly, its trading consistency for performance, its harder do queue things the more constraints you want up front | 03:52 |
| thumper | it means the second attempt will get retried | 03:52 |
| thumper | which can then find an existing one | 03:52 |
| thumper | and say "dumb ass, don't do that" | 03:52 |
| lifeless | really? | 03:52 |
| thumper | yes | 03:52 |
| lifeless | thats ... interesting | 03:53 |
| lifeless | I sure hope we don't do that on apport uploads | 03:53 |
| lifeless | anyhow | 03:53 |
| thumper | the db constraint error causes the request to be retried | 03:53 |
| thumper | I dunno about the apport uploads | 03:53 |
| lifeless | its then causing more work :) | 03:53 |
| lifeless | of a different sort, in a different context. | 03:53 |
| lifeless | I don't mind which way you do it. | 03:53 |
| thumper | heh | 03:53 |
| thumper | you gotta handle it somewhere | 03:54 |
| lifeless | If/when we move to rabbit it will *have* to be done the way I'm describing. | 03:54 |
| lifeless | But you're welcome to do whatever way makes more sense to you know. | 03:54 |
| thumper | well... | 03:54 |
| thumper | not necessarily | 03:54 |
| thumper | we don't want to fire jobs to rabbit for a transaction that'll be aborted | 03:54 |
| lifeless | why not? | 03:54 |
| thumper | so it will have to hook into the transaction somewhere | 03:54 |
| thumper | isn't that obvious? | 03:55 |
| lifeless | Nope. | 03:55 |
| thumper | how about the data not being there? | 03:55 |
| thumper | inconsistent expected stat | 03:55 |
| thumper | e | 03:55 |
| StevenK | thumper: So I'm expecting the test to now raise an IntegrityError if I've added the constraint right? | 03:55 |
| thumper | StevenK: depends how you've done it | 03:55 |
| lifeless | thumper: what data wouldn't be where? and what sort of abort are you expecting? | 03:55 |
| thumper | lifeless: I'm giving examples of why I think it is a bad idea to fire events for something that'll be aborted | 03:56 |
| thumper | I don't have concrete data | 03:56 |
| lifeless | 2PC is pretty nasty perf wise; its almost certainly better to fire-and-check-when-executing. | 03:56 |
| lifeless | BASE rather than ACID | 03:56 |
| thumper | what 2pc? | 03:56 |
| thumper | we don't have two phase | 03:56 |
| lifeless | exactly | 03:56 |
| * thumper walks away | 03:57 | |
| lifeless | thumper: I think we're miscommunicating or something. | 03:57 |
| lifeless | thumper: as I said, right now, whatever makes sense, great. | 03:57 |
| === StevenK changed the topic of #launchpad-reviews to: On call: StevenK || reviewing: mmm, lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Ursinha-afk is now known as Ursinha | ||
| lifeless | thumper: sorry about before, I was needlessly antagonistic; I think I need another neurofen. | 04:14 |
| lifeless | (but we're out) | 04:14 |
| thumper | lifeless: np | 04:16 |
| === Ursinha is now known as Ursinha-afk | ||
| === jtv changed the topic of #launchpad-reviews to: On call: StevenK, jtv || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| lifeless | StevenK: / jtv: who wants this one ? | 06:04 |
| jtv | lifeless: I'll take it | 06:04 |
| jtv | …whatever it is | 06:04 |
| jtv | …assuming it's a Launchpad branch to be reviewed, of under 800 lines of diff. | 06:05 |
| lifeless | hah | 06:05 |
| lifeless | well | 06:05 |
| lifeless | https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/34272 | 06:05 |
| lifeless | diff coming up now | 06:05 |
| jtv | Why do the active-reviews listings just have to get worse and worse? | 06:06 |
| lifeless | so the theme of this patch | 06:06 |
| lifeless | and I don't know how big it is | 06:06 |
| lifeless | is a prepatory patch to logging: | 06:06 |
| lifeless | - resultset iteration | 06:06 |
| lifeless | - email sending | 06:06 |
| jtv | diff's here | 06:06 |
| lifeless | - memcached time | 06:06 |
| lifeless | - rabbit in future | 06:07 |
| lifeless | 713 lines. whew :P | 06:07 |
| lifeless | there is one particular bit of hair. | 06:07 |
| lifeless | I wanted to use request annotations | 06:07 |
| jtv | That's okay—I submitted a 795-line one the other day | 06:07 |
| lifeless | but we can't for anything that scripts use. | 06:07 |
| jtv | Have you looked at how we do this for oopses in scripts? | 06:08 |
| lifeless | so this has that code disabled, references to the bug about this, and instead works in with the existing thing in adapter.py | 06:08 |
| lifeless | jtv: I spent two and a half days trying to make it work | 06:08 |
| jtv | lifeless: it may be lack of context, but I don't understand the comment in line 65 of the diff. | 06:10 |
| jtv | # Requires poking at the API; default is to Just Work. | 06:10 |
| lifeless | its context | 06:10 |
| lifeless | the new API creates a Timeline on demand | 06:11 |
| lifeless | which is particularly well suited for request annotations | 06:11 |
| lifeless | which we will eventually make work in scripts | 06:11 |
| lifeless | but | 06:11 |
| lifeless | checkwatches wants a limitedlist rather than grabbing all the queries | 06:11 |
| lifeless | which is odd, but to keep things scoped I just put in compat code | 06:11 |
| lifeless | I can expand on that, or remove the comment if you like. | 06:12 |
| lifeless | I wanted it to be clear why there isn't an else clause. | 06:12 |
| jtv | lifeless: be aware that you're at least 2½ days ahead of me in this; I'm having a very hard time following you. | 06:12 |
| lifeless | jtv: I started this last tuesday - so 8 days ahead :) | 06:13 |
| lifeless | jtv: I suggest starting with lib/lp/services/timeline/ | 06:13 |
| jtv | Okay, 8 days plus the context I'm not seeing in the diff. In other words: I haven't a frakking clue what you're on about! | 06:13 |
| lifeless | you'll seem some ugliness about _local | 06:14 |
| lifeless | but ignore that; the module and its tests should tell you what its on about | 06:14 |
| jtv | lifeless: lib/lp/services/timeline/__init__.py, diff line 355: "Because this part of [...],"—missing "is" I think. | 06:15 |
| lifeless | yes, will add | 06:16 |
| jtv | lifeless: in requesttimeline.py, diff line 366: if the Timeline in that docstring is a class name, please backquote. | 06:17 |
| lifeless | single or double? | 06:17 |
| jtv | Single. | 06:17 |
| jtv | `Timeline` | 06:17 |
| jtv | And while you're there, please normalize __all__ to multi-line format. | 06:17 |
| jtv | Below that, according to the standards I learned at the time, the XXX should go "XXX RobertCollins 2010-09-01 bug=623199: Undesirable but pragmatic." | 06:18 |
| lifeless | switched around | 06:19 |
| jtv | In diff ll. 389—390, why do you keep the dead code around? | 06:19 |
| jtv | Is it likely that we'll want to get back to it very soon, and that it won't have changed by then? | 06:20 |
| lifeless | yes | 06:20 |
| lifeless | fixing the mess up will be hard | 06:20 |
| lifeless | but it makes a lot of stuff terrible. | 06:20 |
| lifeless | feature flags | 06:20 |
| lifeless | this | 06:20 |
| lifeless | request timing in general | 06:20 |
| lifeless | security contexts | 06:21 |
| lifeless | code clarity all through lib/canonical/launchpad/webapp | 06:21 |
| lifeless | I am generally an advocate of 'if we are deleting it, let the vcs remember' | 06:22 |
| lifeless | but in this case I want folk looking at this code to see how it *should* look. | 06:22 |
| jtv | The two approaches look quite similar to me, just structured slightly differently. | 06:24 |
| lifeless | one is thread local | 06:25 |
| jtv | Ahhh | 06:25 |
| lifeless | which has all sorts of caveats and issues; the other is zope interface based, which is more flexible | 06:25 |
| jtv | That clarifies things. | 06:25 |
| jtv | So two aspects get intermixed here: the way you get to the request, and the way you create the timeline on demand. | 06:28 |
| lifeless | mmm | 06:29 |
| lifeless | I'm not sure I see what you mean | 06:30 |
| jtv | I think I said it wrong. Would this be right? "You changed two different aspects—where the timeline is supposed to be, and how you create it on demand." | 06:31 |
| lifeless | yes | 06:31 |
| lifeless | kindof | 06:31 |
| lifeless | there used to be an api to get the timeline in its less general form | 06:31 |
| lifeless | so this adds an api to get the timeline which lets other places than adapter.py write to it | 06:33 |
| lifeless | what existed before was a readonly api, as it were | 06:33 |
| lifeless | to make the new api easy to use, it creates on demand, but in practice, 'set_request_started' will still be getting used everywhere. | 06:34 |
| lifeless | so until we nuke that function, the new niceness won't really be visible | 06:34 |
| jtv | So what you have is getter/setter functionality which you changed to use a different path to the variable, and on-demand creation which could work unchanged for both the old and the new model. | 06:35 |
| lifeless | fingers crossed :) | 06:36 |
| lifeless | I'd like to get rid of the setter, but its needed to migrate | 06:36 |
| lifeless | chicken and egg, or really big patches. | 06:36 |
| jtv | What confused me is that the dead code differs from the live code on both scores, yet only one of the two matters. | 06:36 |
| lifeless | oh, ah | 06:37 |
| jtv | How about instead of keeping the dead code, you just have an XXX to say "in the future we will keep this in request.annotations['timeline'] instead of webapp.adapter._local.request_timeline"? | 06:37 |
| jtv | Then just keep the structure of the live code, and when the time comes, only change the place where you get/set the timeline. | 06:38 |
| lifeless | so, now I know what you were thinking | 06:38 |
| jtv | It had a lot to do with that 8-day head start. Doesn't necessarily make sense semantically. :) | 06:38 |
| lifeless | these two implementations of these two functions are externally identically equivalent | 06:38 |
| lifeless | that is, get_request_timeline autocreates in both cases | 06:39 |
| jtv | Right, but you're interleaving dead and live code that differ in multiple aspects, for no good reason. Highly confusing! | 06:39 |
| lifeless | and set_request_timeline sets it | 06:39 |
| lifeless | jtv: they don't different in multiple aspects, except one thing: the return from set_, which is just for avoiding duplication because _local isn't a dict. | 06:40 |
| jtv | Well there's that too. | 06:40 |
| jtv | I'd *definitely* avoid that particular difference between the live and the dead code. | 06:40 |
| lifeless | mmm | 06:41 |
| lifeless | I would rather you look at the hairy stuff | 06:41 |
| lifeless | this seems really minor to me. | 06:41 |
| lifeless | I can make set_ return as well in the old code path, but there is no need. | 06:42 |
| jtv | This _is_ really minor, but there shouldn't be such big obstacles to understanding the code. | 06:43 |
| lifeless | I don't understand the obstacles | 06:44 |
| lifeless | I guess | 06:44 |
| lifeless | its two lines | 06:44 |
| jtv | It's more than two lines if you count the code that you'll want to remove later on; you just optimized for minimizing the dead code instead of for clarity. | 06:45 |
| lifeless | I'm optimising for vision | 06:45 |
| jtv | Well my vision is blurred right now! | 06:46 |
| jtv | SCNR | 06:46 |
| lifeless | if the desired code is not visible | 06:47 |
| lifeless | the reasons for the external import are less clear | 06:47 |
| lifeless | the understanding of someone coming and saying 'why is this the way it is' will be less | 06:47 |
| lifeless | if adapter wasn't a terrible terrible global variable pita | 06:48 |
| lifeless | I wouldn't need to do this and noone would need to care. | 06:48 |
| lifeless | brb, getting wood for the fire | 06:49 |
| lifeless | back | 06:54 |
| jtv | OK. Here's something simple you could do to make it clear what the intention of the code is: the "Disabled code path" comments say that the dead code is undesired. If it's the desired code, make that clear. | 06:55 |
| lifeless | here is a patch to increase the info in that file | 06:58 |
| lifeless | http://pastebin.com/5BHK8qhA | 06:58 |
| lifeless | I'll add in the change you just suggested | 06:58 |
| jtv | Thanks. | 06:58 |
| lifeless | # Disabled code path: bug 623199, ideally we would use this code path. | 06:58 |
| _mup_ | Bug #623199: scripts do not establish valid zope partiticipations <Launchpad Foundations:New> <https://launchpad.net/bugs/623199> | 06:58 |
| lifeless | hows that | 06:58 |
| jtv | Much better, but "ideally" suggests that this dead code can stick around indefinitely, until perhaps it no longer makes any sense at all. Either you have an XXX saying that the code must change to this new form and soon, or the dead code is dead weight. | 07:01 |
| lifeless | there is an xxx, right up the top of the file | 07:01 |
| lifeless | can make them all look like that if you like, but they aren't going to vary independently | 07:01 |
| lifeless | only the first one is needed for the XXX report | 07:03 |
| jtv | Who's going to make the change, and when? | 07:03 |
| lifeless | don't know, as soon as I can arrange it | 07:03 |
| jtv | The reason I ask is that the relative value of having the future code spelled out decreases with the time it takes to get to it; the effort involved; and how far removed the person doing it was removed from what you've been working on for the past 8 days. The change you pasted me makes the intent clear, which is a much bigger help. | 07:09 |
| lifeless | When the future code shapes architecture, I disagree about the value diminishing; I think its very helpful to be clear about how things *should* be | 07:10 |
| lifeless | if it was just a different query on a maybe-table, it would be a bit of yagni | 07:10 |
| jtv | lifeless: that's all very nice, but the part you're documenting in the form of dead code is easily reconstructed from the intent. Going the other way is harder: trying to figure out what the dead code is trying to say, and restoring it. | 07:19 |
| lifeless | I don't understand | 07:20 |
| jtv | It's a shame to throw away better code that almost worked, but I think in a case like this it's for the best. | 07:20 |
| lifeless | I don't follow your argument for deleting it | 07:21 |
| jtv | What you have is dead code. As a reader, I have to figure out why the dead code is there, and how it fits into the picture. Then, if I want to fix the issue, I have to figure out how it relates to the live code. The two sit slightly awkwardly together because the same code paths would work for both yet they follow different code paths. But the fundamental change is one that's concisely expressed in one line, and everything else follows from there—y | 07:26 |
| jtv | So now you're making the documentation better, which is good, but it makes the dead code entirely redundant. | 07:27 |
| lifeless | well it doesn't | 07:27 |
| lifeless | a picture paints a thousand words | 07:27 |
| lifeless | the code is exact. Its a direct statement. | 07:28 |
| lifeless | anything else is a pale shadow. | 07:28 |
| jtv | It specifies irrelevant detail and leaves the essence implicit. | 07:28 |
| jtv | The essential difference between the live and the dead code is that the timeline is kept in a different place. | 07:29 |
| lifeless | no | 07:29 |
| lifeless | thats not the essential difference. | 07:29 |
| jtv | Then what is? And can the code make it clearer to me? | 07:30 |
| lifeless | or rather, phrasing the different that way trivialises a huge change | 07:30 |
| lifeless | webapp.adapter uses thread locals | 07:30 |
| lifeless | its not safe with: | 07:30 |
| lifeless | - twisted | 07:30 |
| lifeless | - threadpools | 07:30 |
| lifeless | - async of any sort | 07:31 |
| lifeless | we do a tonne of crazy stuff | 07:40 |
| lifeless | all the work we deferToThread in the twisted apps we have could easily break in subtle ways, at any time | 07:40 |
| lifeless | its very important to me that we should the right way of doing it, even if we can't. | 07:40 |
| jtv | lifeless: No, those are the reasons behind the effort that produced the dead code. The effort itself AFAICT is to keep the timeline in a different place, where it's thread-safe. Moreover, the dead code in the getter and setter keeps all of this entirely implicit, and hides it among secondary differences because the two code paths avoidably use different idioms for on-demand setting. | 07:42 |
| lifeless | do you mean unavoidably? | 07:43 |
| jtv | No, I mean avoidably. | 07:43 |
| lifeless | ones a dict | 07:43 |
| lifeless | one isn't | 07:43 |
| lifeless | I don't see how you can use the same idiom | 07:44 |
| lifeless | any still be writing code idiomatic to the type | 07:44 |
| lifeless | anyhow, this is driving me up the wall. I feel like you want to make it perfect in one particular slant, which destroys its value in other ways. I care about the other ways much more. | 07:45 |
| jtv | I'm glad we're agreed. What I'm saying is: don't struggle so hard to keep 2 lines of dead code around if instead you can make the intent clear enough that the code follows automatically. | 07:46 |
| jtv | If it was _just_ two lines of difference, it'd be defensible. If all these non-essential changes follow, it becomes struggling. | 07:46 |
| jtv | (I'm counting the lines you'd delete in order to make the dead code live) | 07:48 |
| jtv | If you really want to keep them, then I can live with that. Just be clearer about the intent. | 07:50 |
| lifeless | Well, I've added the prose we discussed and rewritten docstrings etc as i pastebinned. | 07:51 |
| lifeless | I'm cooperating on achieving our mutal goals of good code and clean design | 07:51 |
| lifeless | the 'non essential changes' you seem to be referring too, would become longer if those two lines of code are not there. | 07:51 |
| jtv | And even if it doesn't sound like it, I appreciate that and I do care about getting your improvements in. | 07:52 |
| jtv | For the record, I don't object to nonessential changes per se. Only the difficulty of figuring out how the "before" and "after" pictures of the code relate, if they are in the code side by side. | 07:55 |
| jtv | (In the tests I do very much like what you did for the same purpose: add a testable specification of how it _should_ work, even if that can't be implemented yet) | 07:57 |
| jtv | A TimedAction doesn't need a description? | 07:58 |
| lifeless | context? | 07:59 |
| lifeless | there is a class docstring | 07:59 |
| jtv | Sorry, I meant "category." | 08:00 |
| lifeless | its mandatory in the constructor | 08:00 |
| lifeless | its not evaluated by the object | 08:00 |
| lifeless | same for detail | 08:00 |
| jtv | Just curious because I saw one of the tests pass None as a category. | 08:00 |
| lifeless | can change, I was just only-filling-in-what-really-mattered | 08:01 |
| * lifeless does the happy dance, the performance work from yesterday is merged. | 08:03 | |
| jtv | I don't see a particular need to change it myself, just wondering if you might (from a more complete perspective) | 08:03 |
| lifeless | I think its fine | 08:03 |
| jtv | ah, that reminds me of some queries I wanted to zap on one of our slower pages | 08:03 |
| jtv | …but that's for another time | 08:04 |
| jtv | I think my world would be just a tad nicer if right-click menus never immediately placed a potentially harmful action right under the mouse pointer. | 08:05 |
| lifeless | heh yes | 08:05 |
| lifeless | that would be nice | 08:06 |
| jtv | I'm wondering if Timeline.baseline could ever mislead… I could be wrong, but ISTM the timeline could be created explicitly, in which case it's meaningful; or when a first action came along, in which case it's basically the same as self.actions[0].start; or implicitly as a side effect of something indeterminate, in which case it's shaky at best. | 08:08 |
| lifeless | thats a good point | 08:09 |
| lifeless | I think we should a) move to a request.annotations model, and then b) make an IRequestStart hook trigger a timeline existence | 08:09 |
| lifeless | Oh, and this is a reason to put an else block in adapter.py | 08:09 |
| lifeless | where you asked about originally. | 08:09 |
| jtv | Would it be easier perhaps to have a "start timeline" event? | 08:10 |
| lifeless | I don't think so, because the point is that you want requesttimeline to be /subscribed/ not /emitting/ | 08:11 |
| lifeless | + request = get_current_browser_request() | 08:12 |
| lifeless | if request_statements is not None: | 08:12 |
| lifeless | # Requires poking at the API; default is to Just Work. | 08:12 |
| lifeless | - request = get_current_browser_request() | 08:12 |
| lifeless | set_request_timeline(request, Timeline(request_statements)) | 08:12 |
| lifeless | + else: | 08:12 |
| lifeless | + # Ensure a timeline is created, so that time offset for actions is | 08:12 |
| lifeless | + # reasonable. | 08:12 |
| lifeless | + set_request_timeline(request, Timeline()) | 08:12 |
| lifeless | bah, haven't changed the first comment ther,e will do so - but you see the change | 08:12 |
| jtv | I used the wrong term again. Forgive me. A start _action_, so you never have to wonder whether the actions list is empty and don't have to keep a baseline time. | 08:12 |
| lifeless | interesting idea. | 08:13 |
| lifeless | let me think for a minute | 08:13 |
| jtv | Hmm... I think I can find an easier spelling for the if/else you just pasted. :-) | 08:13 |
| lifeless | oh, sure I can pull the set_request call out of it | 08:13 |
| jtv | set_request_timeline(request, Timeline(request_statements)) | 08:14 |
| lifeless | but its a line longer with another local variable | 08:14 |
| jtv | I think ^^ is equivalent to the entire if/else. | 08:14 |
| lifeless | I think that would mislead the eye | 08:14 |
| lifeless | its true that when it is None it is equivalent to not passing it because the implementation uses None as the sentinel | 08:15 |
| lifeless | ok, start actions. | 08:15 |
| lifeless | I think that would need more special cases. | 08:15 |
| lifeless | because: | 08:15 |
| lifeless | log_tuples are derived from the baseline, not the prior action. | 08:15 |
| lifeless | they have to be because people may use a LimitedList (until we sort out checkwatches etc...) | 08:16 |
| jtv | Well, the baseline would just be self.actions[0].start | 08:16 |
| lifeless | and if you derived it recursively, the first action can disappear | 08:16 |
| lifeless | so, if you had a start action, you have to make sure it cannot magically go away when someone uses a LimitedList | 08:16 |
| jtv | oic | 08:16 |
| lifeless | which would require gymnastics | 08:16 |
| lifeless | also | 08:16 |
| jtv | So then by all means don't do that. | 08:17 |
| lifeless | the start is an instantaneous thing, not a blocking event, so its not interesting like other TimedEvents | 08:17 |
| jtv | Maybe though just pull the datetime.datetime.now(UTC) out into a method so it doesn't open you up to maintenance skew? | 08:17 |
| lifeless | what do you mean? | 08:17 |
| jtv | Well actually I have ulterior motives also. I'll explain. | 08:18 |
| jtv | I sometimes get those annoying "hey you're mixing timezone-aware timestamps with timezone-naïve ones" errors. | 08:18 |
| lifeless | yes, they suck. This api is entirely tz aware because of that. | 08:19 |
| jtv | To minimize that distraction while developing, I like having the code that "reads the clock" for one particular item concentrated in one place. | 08:19 |
| jtv | The ulterior one is, of course, that it's a very easy way to mess with timestamps in tests. There's supposed to be better ways to do that, but I haven't gotten around to them yet. | 08:20 |
| jtv | (And by "mess with," I mean "produce predictable") | 08:20 |
| lifeless | sure, a test double time source | 08:20 |
| lifeless | do we have one system wide already? | 08:20 |
| jtv | I don't _think_ so | 08:21 |
| lifeless | Its a little complex to get right, and perhaps a simple mock is better. | 08:21 |
| lifeless | I defintely don't want to go down that hole speculatively here. | 08:21 |
| lifeless | I like the idea, I just think is very conceptually different, and as you can see, only one test would really benefit today. | 08:22 |
| jtv | No… the main thing I'm driving at is "clock-reading code is often best extracted and concentrated." | 08:22 |
| lifeless | I'm agreeing. | 08:23 |
| lifeless | we have it all over the place | 08:23 |
| lifeless | I'm saying 'Rather than start a new trend in this patch, we should do it separately' | 08:23 |
| jtv | Well starting a new trend is a different matter perhaps. | 08:24 |
| lifeless | When I have a strong opinion on the how of it I'll do that in small pieces (like this entire patch is) | 08:26 |
| lifeless | I don't for clock reading code; I can see at least three different approaches | 08:26 |
| jtv | Something else: in the summarize_requests output, maybe just generalize "queries" to "actions," and possibly replace "issued" with "performed" or somesuch? | 08:27 |
| jtv | (No strong feelings on the clock issue here, so do it the way you want it) | 08:27 |
| jtv | lifeless: any ideas on the baseline issue? | 08:32 |
| lifeless | jtv: whats the bsaeline issue ? | 08:32 |
| jtv | Maybe pass it explicitly based on request_starttime? | 08:32 |
| lifeless | that it might be out of sync? | 08:33 |
| lifeless | hmm | 08:33 |
| * lifeless strongly suspects thats a misfeature | 08:33 | |
| lifeless | so right now: | 08:33 |
| jtv | Oh, that's not much good if you create on-demand, is it | 08:33 |
| lifeless | - no tests failed | 08:33 |
| lifeless | - production doesn't play tricks with time | 08:33 |
| lifeless | I think its a yagni, with the patch I pasted in here applied. | 08:34 |
| jtv | If you're always creating the timeline early on, that solves it. | 08:34 |
| lifeless | yeah | 08:34 |
| jtv | Another small thing: in diff lines 229—232, the preferred way to access items in a tuple is to unpack it. I think that'd be a lot clearer in this case. | 08:38 |
| jtv | That's errorlog.py, starting around line 454 or so. | 08:38 |
| lifeless | doing | 08:38 |
| jtv | thx | 08:38 |
| lifeless | - log_tuple = action.log_tuple() | 08:39 |
| lifeless | + start, end, category, detail = action.log_tuple() | 08:39 |
| lifeless | statements.append( | 08:39 |
| lifeless | - log_tuple[:2] + | 08:39 |
| lifeless | - (_safestr(log_tuple[2]), _safestr(log_tuple[3]))) | 08:39 |
| lifeless | + (start, end, _safestr(category), _safestr(detail))) | 08:39 |
| lifeless | I have some habits from writing extremely tight python code | 08:40 |
| jtv | Wonderful. | 08:40 |
| jtv | Believe me, SteveA had to kick some of those habits out of me when I was new... | 08:40 |
| jtv | lifeless: I notice you have a few cases where you look for categories starting with "SQL-". Is that something that's going away? Or perhaps something to move into the timeline class? | 08:40 |
| lifeless | well, I'm not sure these will get kicked out | 08:40 |
| lifeless | but I will certainly be reminding myself when code is inner loop or not | 08:41 |
| lifeless | jtv: its going away, its the old API : I didn't want to have to update tests in this patch | 08:41 |
| jtv | OK | 08:41 |
| lifeless | tests that consume the old API I mean. | 08:41 |
| jtv | Yes, I understand. | 08:41 |
| lifeless | the SQL- prefix in adapter.py will stay, but nothing will filter on it in the main lp codebase | 08:41 |
| lifeless | oopstools may, but the format is a disk format and tricky to evolve | 08:42 |
| lifeless | its a separate concern to the timeline anyway | 08:42 |
| jtv | Then it's not worth messing with. I did have to squint at this one though: | 08:42 |
| jtv | 262+ >>> for action in get_request_timeline( | 08:42 |
| jtv | 263+ ... get_current_browser_request()).actions: | 08:42 |
| jtv | 264+ ... if not action.category.startswith("SQL-"): | 08:42 |
| jtv | 265+ ... continue | 08:42 |
| jtv | 266+ ... print action.detail | 08:42 |
| jtv | Wouldn't that be much clearer as: | 08:42 |
| jtv | >>> timeline = get_request_timeline() | 08:43 |
| jtv | [insert argument I forgot because of the line break in the original] | 08:43 |
| jtv | >>> for action in timeline.actions: | 08:43 |
| jtv | ... if action.category.startswith("SQL-"): | 08:43 |
| jtv | ... print action.detail | 08:43 |
| lifeless | 6 of 1 I think | 08:44 |
| lifeless | changed | 08:44 |
| jtv | Thanks. | 08:44 |
| jtv | lifeless: In webapp/servers.py I see you're replacing sql_statements with nonpython_statements. It could be helpful to standardize on a single term, for grepability etc. | 08:46 |
| lifeless | like actions ? | 08:47 |
| jtv | Yes, that's a good one. :) | 08:47 |
| lifeless | changed | 08:48 |
| jtv | lifeless: thanks. Then, in TestRequestTimeline, there's a pair of comments with --start-- and --end--. Is that idiomatic? I haven't come across it before, and tbh it's the sort of thing I might easily break if there were a few more tests between them. | 08:50 |
| lifeless | its grouping the tests to delete when we fix the bug | 08:51 |
| jtv | Ah! | 08:51 |
| jtv | Obvious once you know it. :) | 08:52 |
| lifeless | I'll add a one-liner reference to the bug on the top one | 08:53 |
| lifeless | # Tests while adapter._local contains the timeline --start--- | 08:53 |
| lifeless | + # When bug=623199 is closed these should be deleted. | 08:53 |
| lifeless | def test_new_request_get_request_timeline_uses_webapp(self): | 08:53 |
| jtv | Great. Maybe just "# Delete when bug=623199 is fixed" for impact? | 08:54 |
| jtv | On our team at least we've been following somewhat different XXX practices: we'll happily add multiple XXX'es for a single bug, to mark all the places that need changing. (They're meant to irk us, after all.) This is one of the places where we'd do that. | 08:55 |
| lifeless | + # Delete when bug=623199 is fixed and the timeline store moved to | 08:56 |
| lifeless | + # annotations. | 08:56 |
| jtv | Even better. | 08:56 |
| lifeless | so, I don't think irking people helps stuff get done | 08:56 |
| lifeless | I think its important to find all the call sites | 08:57 |
| lifeless | and if you see a number that is actually for a fixed bug thats important too | 08:57 |
| lifeless | so - to me - having it in each context that its relevant is crucial | 08:57 |
| lifeless | swamping folk with it is just cruel | 08:57 |
| jtv | Well, it should reflect your intent to fix it. | 08:58 |
| jtv | We were supposed, once upon a time, to pursue XXX'es per se. | 08:59 |
| jtv | Then, the link to a bug became mandatory and we're supposed to pursue bugs. | 08:59 |
| jtv | But yes, now we find that there are always more bugs than you can solve. :) | 08:59 |
| jtv | So maybe you're right and we shouldn't overdo that. | 09:01 |
| jtv | lifeless: on a different matter, in TestTimedAction.test_log_tuple, it _would_ be attractive to get rid of the indeterminacy in the timer. | 09:02 |
| lifeless | jtv: well, it is determinate :) | 09:11 |
| lifeless | the test won't fail because of clock issues | 09:11 |
| lifeless | back shortly, organising foods | 09:11 |
| jtv | lifeless: in TimedAction, log_tuple doesn't meet our standard for method names. But it does meet our standard for property names, and it looks relatively cheap to compute, so maybe just make it a property? | 09:21 |
| lifeless | ugh | 09:40 |
| lifeless | I will have to bring this up, PEP8 is good. | 09:40 |
| lifeless | I will rename it | 09:40 |
| lifeless | (and no, it does computation, so its not suitable for being a property) | 09:42 |
| lifeless | jtv: this is btw, a pathology of the lp review process, I think. | 09:44 |
| jtv | lifeless: which one? | 09:44 |
| lifeless | specification over outcome | 09:44 |
| lifeless | I will expound on it another time | 09:45 |
| jtv | OK, let's get this review done first. | 09:46 |
| jtv | lifeless: typo: s/sancrosant/sacrosanct/g | 09:48 |
| jtv | In OverlappingActionError. | 09:48 |
| lifeless | thanks | 09:48 |
| jtv | Speaking of which, ISTM there are two separate basic cases where you'd run into the non-overlapping rule: failing to mark completion, and nesting. For the former, I'd say the rule is more than just a way to simplify analysis—it detects potential bugs. | 09:50 |
| lifeless | it does, but not a worthwhile class to detect | 09:51 |
| lifeless | because | 09:51 |
| lifeless | those events won't be logged *anyway* in such a case. | 09:52 |
| lifeless | so the developer putting the notifier together will see it immediately. | 09:52 |
| lifeless | I mean, it nice as a bonus, but I'd take nesting, if desired, without argument | 09:52 |
| jtv | Why won't those events be logged anyway? | 09:52 |
| lifeless | something that isn't finished has no OOPS representation today | 09:53 |
| lifeless | you could fudge it | 09:53 |
| jtv | For a query that probably won't matter much, since a failure normally aborts the transaction. But for generalized actions, it may be worth catching if the exception path isn't handled properly. | 09:55 |
| lifeless | I'd like to call YAGNI here | 09:57 |
| lifeless | it can be added | 09:57 |
| lifeless | its not needed now, because every request ends with DB queries which will trigger if the thing before isn't closed off properly. | 09:57 |
| jtv | I don't see the argument for yagni. The functionality is already there, is it not? | 09:58 |
| lifeless | no | 09:59 |
| lifeless | we'd need to add a close-off call, to say 'check that the last thing isn't finished', and call that at the end of the request | 09:59 |
| lifeless | note that 'end of the request' its terribly badly defined today for scripts - see the bug I have been ranting about. | 09:59 |
| lifeless | its not that its terribly hard, its that it has no interesting use case today; you'd need to be using this in a situation that is outside of launchpad core for it matter in any practical sense. | 10:00 |
| === jelmer changed the topic of #launchpad-reviews to: On call: StevenK, jtv, jelmer || reviewing: -, -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| lifeless | and it is a bit hard to do properly, because of 'request' and 'scripts' mixing so badly. | 10:01 |
| jtv | Well in the case where it happens at the end of a request, it'll usually be fairly obvious that the action is there. I'm thinking more of catch-retry cases. | 10:01 |
| lifeless | try: action; except: actionagain | 10:02 |
| lifeless | ? | 10:02 |
| jtv | Well, probably more like except: alternativeaction. | 10:02 |
| lifeless | that will raise already, so no changes are needed, if action() doesn't close off. | 10:02 |
| jtv | That's exactly what I'm saying. | 10:02 |
| lifeless | then I'm horribly confused | 10:03 |
| lifeless | I thought you were asking for a change | 10:03 |
| jtv | No. I'm saying, the check will detect potential bugs. | 10:04 |
| lifeless | ok, so I acked that. I said it wasn't a deliberate feature. | 10:05 |
| jtv | Yes. I'm saying you shouldn't be so apologetic for the limitation. | 10:05 |
| jtv | But anyway, I'm done and approving the branch. | 10:06 |
| lifeless | thank you very much for spending this time on the review. | 10:06 |
| jtv | It's what OCRs are for. | 10:07 |
| lifeless | jtv: so you're clicky-clicking in the webui now ? | 10:14 |
| jtv | lifeless: yes, if you are using that term to describe the sound of my keyboard. | 10:14 |
| lifeless | jtv: oh, are there other changes I need to make? | 10:14 |
| jtv | Fear not. There were a few interruptions slowing me down. | 10:15 |
| * lifeless wants to throw it at ec2 to find out what hidden braindamage I missed | 10:15 | |
| lifeless | I ran the previous iteration through ec2 test | 10:15 |
| jtv | The only thing is "make sure the comments on the dead-code lines aren't easily misread as saying the future code is obsolete." | 10:15 |
| lifeless | which is how i found out about that heinous bug | 10:15 |
| lifeless | jtv: did that | 10:15 |
| jtv | That must've been a painful discovery. | 10:16 |
| lifeless | # Disabled code path: bug 623199, ideally we would use this code path. | 10:16 |
| _mup_ | Bug #623199: scripts do not establish valid zope partiticipations <Launchpad Foundations:New> <https://launchpad.net/bugs/623199> | 10:16 |
| jtv | Ah yes. Good. Now let me finish my review so we can all get on. :) | 10:16 |
| lifeless | jtv: it was unpleasant. Plus I was 1 day into caffeine withdrawal. Not a good combo. | 10:16 |
| jtv | ອາາາາາາາາາາາາາາາາ | 10:17 |
| lifeless | sory, I don't have that glyph | 10:17 |
| jtv | It's "AAAAAAAAAAAAAA!" in Lao. | 10:17 |
| lifeless | :) | 10:18 |
| jtv | Do you have it in Thai? อาาาาาาาาาาาาาาา | 10:18 |
| lifeless | yes | 10:18 |
| lifeless | looks like hills or something | 10:18 |
| jtv | lifeless: it is done | 10:20 |
| jtv | time for a break | 10:20 |
| lifeless | thanks | 10:20 |
| === jtv is now known as jtv-afk | ||
| === StevenK changed the topic of #launchpad-reviews to: On call: jtv, jelmer || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jtv-afk changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Ursinha-afk is now known as Ursinha | ||
| === matsubara-afk is now known as matsubara | ||
| noodles775 | Hi jelmer! I've got one for your queue when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-getByDistroSeries/+merge/34289 | 13:18 |
| === noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jelmer | noodles775: hey! | 13:18 |
| jelmer | noodles775: Of course | 13:18 |
| noodles775 | Great, thanks. | 13:18 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| noodles775 | gar, forgot to push my lint fixes. | 13:22 |
| noodles775 | r9735 is pushed now. | 13:23 |
| === Ursinha is now known as Ursinha-afk | ||
| === jtv1 is now known as jtv | ||
| === Ursinha-afk is now known as Ursinha | ||
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Ursinha is now known as Ursinha-afk | ||
| jtv | EdwinGrubbs, customers! Got time for this one? https://code.launchpad.net/~jtv/launchpad/recife-findtranslationmessage/+merge/34297 | 15:01 |
| EdwinGrubbs | jtv: I can do it | 15:07 |
| jtv | thanks! | 15:07 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | EdwinGrubbs: diff still hasn't showed up. :( | 15:24 |
| EdwinGrubbs | jtv: I ran diff myself against the recife branch. | 15:24 |
| jtv | sorry for the trouble! | 15:25 |
| gmb | jelmer, EdwinGrubbs: Can either of you gents review https://code.edge.launchpad.net/~gmb/launchpad/prompt-user-to-subscribe-to-master-bug-485627/+merge/34302 for me please? | 15:26 |
| EdwinGrubbs | gmb: If jelmer can't do it now, I can do it in an hour or two. | 15:28 |
| gmb | EdwinGrubbs, Thanks. I'll add it to the queue. | 15:28 |
| jelmer | I'll take it then, just wrapping up a review for noodles775. | 15:29 |
| gmb | jelmer, Thanks | 15:29 |
| === gmb changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775, jtv || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | jtv: when makeCurrentTranslationMessage enumerates a list, is it expected that the list contains the plural forms in order of 0, 1, 2, etc.? | 15:30 |
| jtv | EdwinGrubbs: yes | 15:30 |
| jtv | So a translation of a singular and a plural would be [singular, plural] | 15:30 |
| jtv | And a translation to a single-form language, or a translation of a string without a number in it, would be [translation] | 15:31 |
| jtv | But in the API we're standardizing on, those are respectively {0: singular, 1: plural} and {0: translation} | 15:31 |
| EdwinGrubbs | jtv: findTranslationMessage() still has a potranslations parameter that appears to be unused since you set the potranslations variable in the method. | 15:57 |
| gmb | jelmer, I need to step out for a while, so please feel free to ask any questions on the MP and I'll reply when I return. | 16:03 |
| jelmer | gmb: *nod* | 16:04 |
| * gmb exits stage right | 16:04 | |
| jtv | EdwinGrubbs: whoops—well spotted. That was an intermediate attempt at solving the multi-format question. | 16:09 |
| EdwinGrubbs | jtv: ok, that was my only concern. r=me | 16:10 |
| jtv | \o/ thanks | 16:10 |
| EdwinGrubbs | jelmer: are you reviewing my branch already? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-597738-bug-service-status/+merge/34250 | 16:15 |
| noodles775 | Thanks for the review jelmer | 16:18 |
| === matsubara is now known as matsubara-lunch | ||
| jelmer | EdwinGrubbs: Yes, I started on it before you were here but then switched to reviewing noodles' | 16:21 |
| EdwinGrubbs | jelmer: ok, I was going to ask if you could do that now. I'll take gmb's branch. | 16:22 |
| jelmer | EdwinGrubbs: Oh, ok. That works for me as well. | 16:23 |
| jelmer | EdwinGrubbs: btw, it's not really an issue for the review (seems like a formatting issue) but did you see your branch has a conflict? | 16:28 |
| EdwinGrubbs | jelmer: I can imagine that since I'm dependent on a branch that may have landed recently with some minor changes. | 16:30 |
| === salgado is now known as salgado-lunch | ||
| EdwinGrubbs | gmb: r=me | 16:55 |
| === Ursinha-afk is now known as Ursinha | ||
| === benji is now known as benji-lunch | ||
| gmb | EdwinGrubbs, Thanks. | 17:15 |
| === matsubara-lunch is now known as matsubara | ||
| === salgado-lunch is now known as salgado | ||
| === benji-lunch is now known as benji | ||
| === deryck is now known as deryck[lunch] | ||
| === gary_poster_ is now known as gary_poster | ||
| === deryck[lunch] is now known as deryck | ||
| bryceh | On call: jelmer, Edwin || reviewing: noodles775, jtv || queue: [gmb, bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | 18:54 |
| === bryceh changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775, jtv || queue: [gmb, bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bryceh | dur | 18:55 |
| leonardr | salgado, can i ask you to take one more look at my branch (http://pastebin.ubuntu.com/486932/). it makes the changes you requested and fixes some test failures, partly by moving more code into the *Public interfaces | 20:44 |
| salgado | leonardr, sure | 20:48 |
| salgado | <require | 20:48 |
| salgado | permission="zope.Public" | 20:48 |
| salgado | - interface="canonical.launchpad.interfaces.IOAuthAccessTokenVerification"/> | 20:48 |
| salgado | you can use <allow interface="..."> instead of <require permission="..." interface="..."> | 20:49 |
| leonardr | it's the same when the permission is zope.public? | 20:49 |
| salgado | yes | 20:50 |
| salgado | what I meant is that | 20:50 |
| salgado | you can use <allow interface="..."> instead of <require permission="zope.Public" interface="..."> | 20:50 |
| salgado | if you want any other permission, then you should use <require> | 20:50 |
| leonardr | ok | 20:51 |
| === jelmer changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: jtv || queue: [gmb, bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Ursinha is now known as Ursinha-afk | ||
| jtv | I don't think EdwinGrubbs is really still reviewing my branch. :) | 20:52 |
| === jtv changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [gmb, bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jelmer | ah, ok :-) | 20:52 |
| lifeless | jtv: hi | 20:52 |
| jtv | hi | 20:52 |
| lifeless | jtv: a) go to bed :) b) I had two tiny tests that needed a s/queries/activies/ that i missed | 20:53 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | lifeless: aye-aye. :-) | 20:53 |
| === jtv is now known as jtv-zzz | ||
| lifeless | :) | 20:53 |
| salgado | leonardr, I've asked a few other questions on the m-p, in case you didn't see | 21:30 |
| leonardr | salgado, checking | 21:30 |
| leonardr | salgado: the permission isn't changing in this diff. the permission changed last week, and i only noticed the test failure today | 21:35 |
| leonardr | the permission changed last week because we couldn't come up with a way to make the access level be 'write' for oauthtoken objects and 'read for everything else | 21:35 |
| salgado | ok | 21:36 |
| leonardr | i needed to remove the security proxy as soon as I changed the permission for the oauth token, but again, i only saw the test failure today | 21:36 |
| leonardr | salgado, i've responded on the mp | 21:46 |
| EdwinGrubbs | jelmer: I have responded to your review. | 21:47 |
| EdwinGrubbs | bryceh: did you want a review of your branch besides the db reviews by stub and lifeless? | 21:51 |
| EdwinGrubbs | bryceh: I saw that you put your name in the review queue, but I didn't know if there was a different branch that I'm not seeing that you want reviewed. | 21:52 |
| jelmer | EdwinGrubbs, still there? | 22:06 |
| EdwinGrubbs | jelmer: yep | 22:06 |
| jelmer | EdwinGrubbs: Great, I'll have a look at the updated MP. | 22:07 |
| bryceh | EdwinGrubbs, thanks. The branch is stripped down to just db schema changes, so perhaps is just review of it by stub and lifeless sufficient, or should it receive regular review as well? | 22:17 |
| EdwinGrubbs | bryceh: it's fine if they just do the db review, especially since you already had a pre-imp call with somebody from the bugs team. I'll take your name out of the queue, since stub and lifeless aren't oncall reviewers. | 22:20 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bryceh | EdwinGrubbs, ok thanks | 22:20 |
| lifeless | actually I am | 22:34 |
| lifeless | I think I'm on this morning | 22:34 |
| * lifeless checks timetable | 22:34 | |
| === lifeless changed the topic of #launchpad-reviews to: On call: Edwin, Lifeless || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| lifeless | stub is ocr on monday asiapac fwiw | 22:35 |
| === matsubara is now known as matsubara-afk | ||
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Lifeless || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Ursinha-afk is now known as Ursinha | ||
| === salgado is now known as salgado-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!