| jtv | lifeless, stub: requesting db review for the schema changes in our Recife feature branch. | 07:07 |
|---|---|---|
| jtv | https://code.launchpad.net/~launchpad/launchpad/recife/+merge/41426 | 07:08 |
| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| stub | jtv: Having a look now. On the phone before. | 07:29 |
| stub | jtv: So my initial response is similar to lifeless. I don't like that we are embedding ubuntu into our data model when Launchpad is supposed to be distro agnostic. But I also understand that this might be the most practical solution for us for the next few years at least. | 07:35 |
| stub | jtv: Or maybe I'm overreacting - when LP is supporting translating multiple distributions, will they all be Ubuntu derivatives? | 07:37 |
| stub | I can't help but look at TranslationMessage and notice the msgstr1..msgstr5 columns might fit nicely into the Cassandra model ;) | 07:41 |
| stub | jtv: Why is this split into two patches? | 07:44 |
| stub | Cool - ALTER INDEX supports renaming now. I think you used to have to use ALTER TABLE to rename indexes. | 07:45 |
| jtv | stub: the msgstr0…msgstr5 would fit nicely into an array, I guess. | 08:02 |
| * jtv catches up on review | 08:03 | |
| jtv | stub: the ubuntu-specific naming of the column is something we went over at the time. The upshot was that it followed logically from our strategic direction. We'll also support Ubuntu derivatives, but we never really supported translating completely separate distros on a par with Ubuntu anyway. | 08:07 |
| stub | Cool. Matches what I was thinking. Does that make is_current_distro a better name, or would that just be confusing? I don't think we use 'ubuntu' or other celebrity names in the data model at the moment. | 08:09 |
| stub | I'll let either pass - just bringing up the point. | 08:10 |
| stub | How does this interact with patch-2208-28-1 and patch-2208-29-0 (the other translation message patches from danilo)? | 08:11 |
| stub | (15:09:01) stub: Cool. Matches what I was thinking. Does that make is_current_distro a better name, or would that just be confusing? I don't think we use 'ubuntu' or other celebrity names in the data model at the moment. | 08:22 |
| stub | (15:10:11) stub: I'll let either pass - just bringing up the point. | 08:22 |
| stub | (15:11:55) stub: How does this interact with patch-2208-28-1 and patch-2208-29-0 (the other translation message patches from danilo)? | 08:22 |
| jtv | stub: sorry, was off updating the MP to respond to Robert. | 08:25 |
| jtv | I'll have a look at those other patches now. AFAICT we have two patches because they were written at very different times, by different people, and for different purposes—I don't see any reason against merging them except basic laziness. | 08:26 |
| jtv | stub: 2208-28-1 is a cleanup after a model change that was an enabler for the recife branch in terms of managing the complexity. It drops a column, and replaces some indexes that referenced it (with a lot fewer indexes, knowing Danilo's style of indexing). | 08:28 |
| jtv | Ah, the actual "drop column" happens in 2208-29-0. | 08:29 |
| stub | So we need to be careful when your branch lands? As I understand it, danilo's patches needed testing on staging | 08:29 |
| jtv | stub: yes, that's one of the very next things on our plate—delayed only by staging updates. | 08:30 |
| * stub just approved the review | 08:30 | |
| jtv | Thanks! | 08:30 |
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === henninge changed the topic of #launchpad-reviews to: On call: - || Reviewing: || queue: [danilo] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jelmer is now known as Guest24864 | ||
| === Guest24864 is now known as jelmer | ||
| === mrevell is now known as mrevell-lunch | ||
| === matsubara is now known as matsubara-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| jtv | Hi abentley! Danilo's actually not here, so henninge and I are trying to get his branch through. (We're sort of on the clock). | 14:20 |
| jtv | Assuming you'll be ocr'ing today, if you have any problems with the branch, please mention them to either of us. | 14:20 |
| abentley | jtv: Hi. Okay. | 14:20 |
| abentley | jtv: henninge is also OCR. If he approves, I wouldn't normally get involved. | 14:20 |
| jtv | (Sorry to pounce on you first thing—just making sure we don't lose the opportunity due to timezone differences) | 14:21 |
| === jaycee is now known as jcsackett | ||
| jtv | abentley: yes, henninge's finishing something for the same deadline though. | 14:21 |
| jtv | (Plus, we don't want toget _too_ incestuous with these reviews—a little understanding can be a bad thing :-) | 14:22 |
| abentley | jtv: Okay. | 14:22 |
| henninge | abentley: thanks | 14:22 |
| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: || queue: [danilo, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| abentley | jtv: I don't really understand what this code does. | 14:33 |
| jtv | abentley: ah, I see there's a bit of conceptual background missing. | 14:34 |
| jtv | We're changing our data model. Every TranslationMessage currently has two flags: "is_current" and "is_imported." That means that for every translation in Launchpad, we're basically tracking what Launchpad considers the current translation, and what the latest translation is that we saw from upstream. | 14:35 |
| jtv | In the new data model, we merge those. | 14:35 |
| jtv | So we'll share the same TranslationMessage between a project and an Ubuntu package (where appropriate), and we change the flags to "is_current_ubuntu" and "is_current_upstream" respectively. | 14:36 |
| jtv | That doesn't change much for Ubuntu translations, but it changes everything for the project ("upstream") translations. | 14:36 |
| abentley | So is_current now means is_imported, and the old is_imported should go away? | 14:37 |
| jtv | Not that simple, alas. | 14:37 |
| abentley | jtv: What is the code at 65-72 doing? | 14:38 |
| jtv | For Ubuntu translations, you could say that is_current merely gets renamed to is_current_ubuntu whereas is_imported is both renamed to is_current_upstream and changed to get its information from a project in LP, rather than Ubuntu imports with a special "this is from upstream" flag set. | 14:38 |
| jtv | For upstream translations, the information that used to come from is_imported is basically useless and merely confuses and annoys our users. | 14:38 |
| jtv | But the important thing for upstream translations, what used to be is_current, will now come from the is_current_upstream flag. Which is exactly not the right one. | 14:39 |
| jtv | So this script copies, for upstream translations, the old is_current flag to the new is_current_upstream flag. | 14:39 |
| jtv | Now for your question. :-) | 14:39 |
| === salgado is now known as salgado-lunch | ||
| jtv | That looks like a piece of puzzling optimization. | 14:40 |
| jtv | (Go away windmill, I'm trying to concentrate!) | 14:41 |
| jtv | It's definitely not following our coding standards for indentation, but it's a one-off script so that's not likely to matter so much. | 14:42 |
| abentley | jtv: What is is_current_upstream? It does not appear in the script. | 14:43 |
| jtv | abentley: it gets confusing… this script is to be landed on devel, and run _before_ we migrate. So it refers to the old-style flag names. | 14:44 |
| jtv | So is_imported == is_current_upstream. | 14:44 |
| jtv | Anyway, about that strange-looking query: | 14:45 |
| jtv | it looks as if it gets "all the translationmessages that share the same set of potmsgsets." | 14:45 |
| jtv | That's one of those things we do a lot with message-sharing scripts. | 14:45 |
| jtv | So I guess it's not a piece of optimization at all, just a kind of back-and-forth join. | 14:46 |
| jtv | TM → POTMsgSet ← TM | 14:46 |
| jtv | The _reason_ it does this is that there is a unique constraint involving the is_imported flag. | 14:47 |
| jtv | The flag basically means "this is the blessed upstream translation, for this potmsgset and language." There can't be two of those for the same potmsgset & language, so the old ones get cleared first. | 14:48 |
| abentley | jtv: I see. | 14:49 |
| abentley | jtv: So it seems that the ones that are being set as is_imported are the ones that were formerly TranslationMessage.is_current == True, TranslationMessage.is_imported == False | 14:52 |
| abentley | jtv: So it seems that the ones that are being set as is_imported are the ones that were formerly TranslationMessage.is_current == True, TranslationMessage.is_imported == False | 14:54 |
| jtv | abentley: the is_current part of that is the really important information; the "is_imported == False" looks like it's just there to save on replication. | 14:55 |
| jtv | Setting it to True where it's already True won't change anything, but it will cause the row to be re-replicated which is a waste of time. | 14:56 |
| abentley | jtv: Is it possible that 65-72 could be applied to something that was is_current + is_imported, setting it to False when it should be True? | 14:57 |
| * jtv ponders | 14:58 | |
| abentley | jtv: And then since we're ignoring is_current + is_imported, it wouldn't get reset correctly? | 14:58 |
| jtv | abentley: I don't think so—it'd be nice to have a helpful docstring there but it looks like the intent of the method is to set the flag to True exactly on the given TMs. | 14:59 |
| jtv | First it disables just those that are in the way, then it enables those that it's supposed to set. | 14:59 |
| abentley | jtv: And it's not supposed to set those that are is_current + is_imported. | 15:00 |
| jtv | abentley: where is the check for is_imported == False, actually? | 15:00 |
| abentley | 120+ | 15:01 |
| jtv | I see, thanks. | 15:01 |
| jtv | Well ISTM those TMs will never get passed to the TranslationMessageImportedFlagUpdater. | 15:02 |
| jtv | Let me think about that some more. | 15:02 |
| jtv | (To see if there's any other way in which they could ever be passed to it) | 15:03 |
| === matsubara-lunch is now known as matsubara | ||
| jtv | abentley: man, you're good. You spotted a bad one. | 15:07 |
| abentley | jtv: Glad I can help. | 15:08 |
| jtv | This is what happens: | 15:08 |
| jtv | The code seems to assume that there can be only one is_imported TM per POTMsgSet. | 15:08 |
| jtv | In which case, it's perfectly valid to say "I'm doing each TM once, and for each, I'll clear any previous is_imported flags first." | 15:09 |
| jtv | But what of another TM for the same POTMsgSet and a different language, that's already been processed? | 15:10 |
| jtv | It's had its flag set in a previous iteration, but now gets it callously cleared again "to make room" for a TM it doesn't even compete with. | 15:11 |
| abentley | jtv: So are we just missing a condition on language? | 15:12 |
| jtv | If only! | 15:12 |
| jtv | AFAICT there's a similar situation with divergence (which is the case where TM.potemplate is non-null). | 15:12 |
| jtv | So we'd need some extra join conditions: tm1.language = tm2.language and tm1.potemplate is not distinct from tm2.potemplate. | 15:13 |
| abentley | jtv: it sounds like you need some extra tests as well, to check that those cases are handled correctly. | 15:16 |
| jtv | abentley: absolutely. I'm very glad you spotted this. I'll work on that next. I'll have to submit a separate MP once I make changes to danilo's branch. | 15:18 |
| abentley | jtv: You can resubmit this merge proposal, and change the source branch to your own. | 15:18 |
| jtv | ah! | 15:18 |
| jtv | Thanks for the tip. | 15:18 |
| abentley | jtv: np. I just implemented that recently. | 15:21 |
| jtv | This is another one of those subtle reasons why reviews are useful. :) | 15:21 |
| jtv | It's our water-cooler. | 15:21 |
| jtv | abentley: I'll want to be sharp for this change… I guess I'd better finish it tomorrow to avoid stumbling. | 15:22 |
| abentley | jtv: A few style things: at 18, I'd prefer to import from storm.locals, and do a multi-line import. | 15:22 |
| jtv | abentley: ah yes, I'll fix that. | 15:22 |
| jtv | abentley: that one's done, go ahead. :) | 15:23 |
| abentley | 316 could be reformatted to a normal multi-line import. | 15:23 |
| jtv | Done. | 15:24 |
| abentley | At 145, I think tm_loop would be better as import_flag_loop. | 15:24 |
| abentley | or update flag loop. | 15:24 |
| abentley | something about what it does, not what it affects. | 15:25 |
| jtv | OTOH this isn't just a loop that _affects_ TMs; it's a loop _over_ TMs. So it can be insightful to express that. | 15:25 |
| jtv | I'm pretty sure the docstring for that last class is wrong. :) | 15:26 |
| abentley | jtv: to me a loop _over_ TMs is "what it affects". | 15:26 |
| jtv | It's that, but it's also structure. | 15:27 |
| jtv | If you feel strongly about it though, I'll change it. It's the least I owe you. :-) | 15:29 |
| abentley | jtv: No, not strongly. | 15:29 |
| abentley | jtv: Could I get a docstring on _updateTranslationMessages though? | 15:29 |
| jtv | (Pushing updated docstring for that class at the end) | 15:29 |
| jtv | With pleasure! | 15:29 |
| * jtv types | 15:30 | |
| jtv | abentley: → | 15:33 |
| jtv | (22:29:57) jtv: With pleasure! | 15:33 |
| jtv | (22:29:59) ***jtv types | 15:33 |
| abentley | jtv: I think that's it. | 15:33 |
| === abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | abentley: a showcase for meaningful reviews if ever I saw one. Thanks again. | 15:34 |
| abentley | jtv: You're welcome. | 15:34 |
| abentley | jtv: Every time I see a branch with "recife" in the name, I think you guys can't spell recipe. | 15:35 |
| jtv | heh | 15:35 |
| === henninge changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | abentley: I've looked at jtv's branch already, so I'll do a proper review. | 15:37 |
| abentley | henninge: Great, thanks. | 15:37 |
| henninge | jtv: Is there a good reason to expose validateTranslations in IPOTMsgSet? | 15:37 |
| jtv | henninge: yes, the browser code will call it. | 15:38 |
| henninge | ah, permissions ;) | 15:40 |
| henninge | jtv: r=me | 15:44 |
| jtv | thanks henninge | 15:44 |
| jtv | Anyone up for a largish Translations review? I took enough advantage of our OCR already, so would appreciate a volunteer! | 16:10 |
| jtv | https://code.launchpad.net/~jtv/launchpad/recife-kill-updatetranslation-browser/+merge/41483 | 16:11 |
| henninge | jtv: Why don't you go to bed and I look at that mp and we talk about it in the morning (my morning). I 'll make sure I'll be her much earlier than today. | 16:25 |
| jtv | henninge: great, thanks. I'm just wrapping up the day here. | 16:25 |
| allenap | abentley: Hi, can you review a change to ec2test please? https://code.launchpad.net/~allenap/launchpad/update-ec2-image/+merge/41485 | 16:28 |
| abentley | allenap: VALID_AMI_OWNERS should come after EC2Account in the __all__ | 16:30 |
| allenap | abentley: Oh yes :) | 16:31 |
| abentley | allenap: You can avoid the sort, groupby and for loop by using setdefault. e.g. setdefault(result, revision, []).append(image) | 16:35 |
| abentley | allenap: I mean, result.setdefault(revision, []).append(image) | 16:35 |
| allenap | abentley: I still want to sort, so I'll still need to sort by revision and unpack. | 16:39 |
| abentley | allenap: I think it'll still be simpler, though. | 16:41 |
| === jelmer is now known as Guest34932 | ||
| === Guest34932 is now known as jelmer | ||
| allenap | abentley: D'oh, yes, you're right. I've also used a defaultdict. | 16:52 |
| abentley | allenap: Cool. | 16:53 |
| === salgado-lunch is now known as salgado | ||
| === deryck is now known as deryck[lunch] | ||
| === benji is now known as benji-lunch | ||
| === deryck[lunch] is now known as deryck | ||
| === benji-lunch is now known as benji | ||
| === matsubara is now known as matsubara-afk | ||
| === salgado is now known as salgado-afk | ||
| === abentley changed the topic of #launchpad-reviews to: On call: - || Reviewing: || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!