| rockstar | thumper, ping. | 02:43 |
|---|---|---|
| thumper | rockstar: hey hey | 02:43 |
| rockstar | thumper, unping. I need to check to make sure my assertion is correct before making it. | 02:44 |
| rockstar | thumper, NOW I think I'm ready to land this branch. I've taken out the tree creation (at one point I was running the job, so it needed a tree) | 03:47 |
| thumper | rockstar: is it pushed? | 03:48 |
| rockstar | thumper, yessir. A bit ago (I got distracted) | 03:49 |
| thumper | rockstar: https://code.launchpad.net/~thumper/launchpad/more-product-series-permissions/+merge/16888 | 03:52 |
| rockstar | thumper, wait, I have to scratch your back for to scratch mine? :) | 03:53 |
| thumper | rockstar: you don't *have* to... | 03:53 |
| rockstar | thumper, I'm kidding. Looking now. | 03:54 |
| rockstar | thumper, so this allows us to delete import branches that are the series branch? | 03:54 |
| thumper | rockstar: yes | 03:55 |
| rockstar | Awesomeness. | 03:55 |
| thumper | rockstar: now I have a few questions | 03:55 |
| rockstar | thumper, shoot. | 03:55 |
| thumper | rockstar: why create two branches in the test? | 03:55 |
| thumper | rockstar: if you are only testing that newer ones aren't deleted, you don't need two branches | 03:55 |
| thumper | branch jobs | 03:55 |
| thumper | just one | 03:56 |
| thumper | is enough | 03:56 |
| rockstar | Hm, okay. | 03:56 |
| thumper | then you can remove the first assert | 03:56 |
| thumper | also why use transaction.commit() | 03:56 |
| thumper | is that really necessary> | 03:56 |
| thumper | ? | 03:56 |
| thumper | you have no comment at the top of the test | 03:56 |
| thumper | you have two lines between the tests | 03:56 |
| rockstar | thumper, yes, otherwise it blows up apparently. | 03:56 |
| thumper | and I don't think you need tempfile anymore | 03:56 |
| thumper | apparently or certainly? | 03:56 |
| rockstar | thumper, it was blowing up otherwise. | 03:57 |
| thumper | with what error? | 03:57 |
| thumper | I don't see why you should need the commit | 03:57 |
| rockstar | thumper, lemme try. | 03:58 |
| thumper | mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/revision-karma-fix/+merge/16890 | 04:02 |
| thumper | mwhudson: ta, using critical | 04:08 |
| rockstar | thumper, yea, looks like the transaction.commit is needed. | 04:22 |
| thumper | rockstar: hmm | 04:23 |
| thumper | ok | 04:23 |
| thumper | rockstar: have you fixed the other bits? | 04:23 |
| rockstar | thumper, no idea, but the other tests in the TestCase do it as well. | 04:23 |
| rockstar | thumper, other bits fixed, pushing now. | 04:24 |
| thumper | ok | 04:24 |
| rockstar | thumper, diff is updated. | 04:29 |
| thumper | rockstar: replied, a few more things to fix | 04:34 |
| rockstar | thumper, I think I got them all this time. | 04:42 |
| * rockstar REALLY wants to send this branch off the ec2 | 04:43 | |
| rockstar | thumper, going home from the coffee shop, back soon (hopefully to submit to PQM) | 04:47 |
| thumper | ok | 04:47 |
| thumper | rockstar: you are good to go now | 04:48 |
| rockstar | thumper, sweet, thanks. | 05:01 |
| === jtv changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === jtv is now known as jtv-afk | ||
| === stub1 is now known as stub | ||
| === jtv-afk is now known as jtv | ||
| jtv | stub: I followed up on your db review yesterday... is it enough to take you from Needs Information to Approved? | 10:29 |
| stub | jtv: updated | 10:44 |
| jtv | ta | 10:44 |
| === salgado-afk is now known as salgado | ||
| === matsubara-afk is now known as matsubara | ||
| al-maisan | hello jtv, are you still reviewing? | 11:58 |
| jtv | al-maisan: indeed! | 11:58 |
| al-maisan | Great! Would you mind taking a look at https://code.edge.launchpad.net/~al-maisan/launchpad/xx-next-builder-502927/+merge/16896 ? | 11:58 |
| jtv | gimme | 11:58 |
| jtv | looking... | 11:58 |
| al-maisan | thanks :) | 11:58 |
| jtv | oh nice... will this one also generalize BuildQueue.required_build_behavior? | 11:59 |
| jtv | seems no... but progress is progress :) | 11:59 |
| al-maisan | jtv: not really .. this is still about job dispatch time estimation | 12:00 |
| al-maisan | step n-3 | 12:00 |
| jtv | Oh, I see you got builder_key un-nested... I like it better this way | 12:00 |
| jtv | (but didn't say anything last time because IIRC you had nested it based on another reviewer's suggestion) | 12:00 |
| al-maisan | yes, it was jml's suggestion I believe | 12:01 |
| al-maisan | now builder_key() is used in more than one place | 12:01 |
| jtv | damn his morning croissant | 12:01 |
| al-maisan | .. and the crumbles :) | 12:02 |
| jtv | But where is _minTimeToNextBuilderAvailable actually called? | 12:02 |
| al-maisan | jtv: nowhere yet | 12:02 |
| jtv | (tests aside, of course) | 12:02 |
| al-maisan | that will come in one of the next branches | 12:02 |
| jtv | OK | 12:02 |
| jtv | Then bear in mind that a method name should begin with a verb | 12:03 |
| al-maisan | the branch this is taken from is almost 2K LOC | 12:03 |
| al-maisan | ah .. a verb | 12:03 |
| jtv | maybe "estimate"? | 12:03 |
| al-maisan | I can fix that | 12:03 |
| al-maisan | "estimate" is nice .. I like that. | 12:03 |
| jtv | instead of "min" (because the docstring suggests this is an estimated time, not a mininum) | 12:03 |
| al-maisan | good idea! | 12:04 |
| jtv | Also, "Get" is one of those over-generic and over-used words... Why say "Get estimated time" when you're really producing one? Same verb can serve there. :-) | 12:04 |
| al-maisan | hmm .. fair enough. | 12:05 |
| jtv | Also, mixed_mode is a bit cryptic. Can you document it in the docstring? | 12:05 |
| al-maisan | jtv: I can but the big comment at the beginning of the method is all about the "mixed mode" | 12:06 |
| jtv | al-maisan: I look at the comments when I want to know more about the implementation, but the docstring should give me the "This Method for Dummies" meaning of the parameter. | 12:07 |
| jtv | How about processor_dependent? | 12:08 |
| jtv | or even, is_processor_specific? | 12:08 |
| jtv | As the law goes, two things are hard in computer science: cache invalidation and naming things. | 12:09 |
| al-maisan | jtv: I can add the explanation in the doc string. | 12:10 |
| al-maisan | .. and yeah, naming things is not easy, I agree :) | 12:11 |
| al-maisan | "mixed_mode" really expresses the meaning best IMHO | 12:11 |
| al-maisan | think about a queue with jobs | 12:11 |
| al-maisan | what this method needs to know is whether there are jobs ahead of the job of interest (JOI) that are processor independent | 12:12 |
| al-maisan | if yes, then all builders should be considered | 12:12 |
| jtv | isn't that "mode" a property of the BuildQueue object? | 12:13 |
| al-maisan | jtv: not quite each job has a BuildQueue | 12:13 |
| jtv | Right, that's why I'm asking. | 12:14 |
| al-maisan | BuildQueue should actually be named BuildQueueJob | 12:14 |
| jtv | The BuildQueue's job can be architecture-specific or non-architecture specific... isn't that what determines which value will be passed here? | 12:14 |
| al-maisan | what is being passed in here is: are there any jobs ahead of me that are processor independent? | 12:15 |
| al-maisan | ahead of me in the sense that they have a higher score and .. | 12:15 |
| al-maisan | .. should be dispatched before me | 12:15 |
| al-maisan | me being the build queue job of interest | 12:16 |
| jtv | al-maisan: maybe it would help me to understand why you need the two "modes." Are you trying to get an estimate of when this particular job can start running (assuming that it's at a front of the queue)? | 12:17 |
| al-maisan | again, think of a queue of jobs | 12:18 |
| al-maisan | jtv: please give me few minutes to come up with a clarifying example | 12:19 |
| jtv | al-maisan: it would help me a lot right now to have an idea of what this method will be used for | 12:20 |
| al-maisan | OK | 12:20 |
| al-maisan | the question this method answers is: what is the shorted time until a builder capable of running the JOI becomes available? | 12:21 |
| al-maisan | jtv: s/shorted/shortest/ | 12:22 |
| jtv | but then my previous question stands: isn't whether the job is architecture-dependent or not a property of the BuildQueue object (or its associated Job etc.)? | 12:22 |
| al-maisan | yes, that's what's encoded in the 'my_processor' local variable | 12:23 |
| al-maisan | but | 12:23 |
| al-maisan | there's (at least) one other scenario where that is not sufficient | 12:24 |
| al-maisan | job queue [amd_job_1, proc_independent_job_2, x86_job_3, x86_job_4] | 12:25 |
| al-maisan | let's assume x86_job_4 is the JOI | 12:25 |
| al-maisan | in that case proc_independent_job_2 and x86_job_3 compete for its builder pool | 12:26 |
| al-maisan | but not amd_job_1 | 12:26 |
| al-maisan | so, what I need to know is: how long is the delay until the head of the queue of mutually competing jobs can be dispatched | 12:28 |
| jtv | (btw otp sorry) | 12:29 |
| al-maisan | jtv: actually, I just realized that the method may not be doing what it's supposed to do. | 12:30 |
| al-maisan | jtv: please ignore this merge proposal until I come back to you. | 12:30 |
| === mrevell is now known as mrevell-lunch | ||
| * jtv finally gets off phone | 12:42 | |
| jtv | al-maisan: the entire MP? And we were still on the first line of diff! | 12:42 |
| jtv | new record for me ;-) | 12:43 |
| al-maisan | jtv: you are asking good questions :) | 12:43 |
| jtv | "I could tell from the way his first two characters rendered in my browser that there was a bug somewhere" | 12:44 |
| jtv | meanwhile, I'll go have a standup :) | 12:44 |
| al-maisan | jtv: thanks for looking at this anyway :) | 12:44 |
| jtv | no worries, it's what we're working on! | 12:44 |
| === mrevell-lunch is now known as mrevell | ||
| al-maisan | hello gmb, I know it's not your OCR day but I have a branch that needs to land today. Would you be in a position to take a look at it? It's 611 lines long but the bulk of it are tests. | 14:11 |
| al-maisan | here's the diff: http://pastebin.ubuntu.com/352336/ | 14:12 |
| gmb | al-maisan: Sure. | 14:13 |
| gmb | al-maisan: Is there a merge-proposal for this? | 14:13 |
| al-maisan | gmb: I really appreciate your help .. the MP is here: https://code.edge.launchpad.net/~al-maisan/launchpad/xx-next-builder-502927/+merge/16896 | 14:13 |
| gmb | al-maisan: No worries :). I'll grab a cup of tea and then take a look. | 14:14 |
| al-maisan | gmb: if you want to go through examples, I have a picture of some scenarios here: https://devpad.canonical.com/~muharem/IMG_0275.JPG | 14:14 |
| gmb | al-maisan: Thanks. | 14:14 |
| al-maisan | we can also have a voice call .. whatever suits you. | 14:17 |
| gmb | al-maisan: Bear with me; I'll ping you when I need more information :) | 14:28 |
| al-maisan | gmb: great :) | 14:29 |
| === salgado is now known as salgado-lunch | ||
| === abentley changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue [abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === fjlacoste is now known as flacoste | ||
| gmb | al-maisan: So, what happens if our 120 second wild guess is wrong? Anything bad? | 15:21 |
| al-maisan | gmb: not really .. the estimate will not be as good as we'd like. | 15:21 |
| al-maisan | the dispatch time estimate that is | 15:22 |
| gmb | al-maisan: Okay, cool. I think that the comment in the SQL should be updated to reflect that it's not going to make horrible things happen. | 15:22 |
| al-maisan | gmb: fair enough, can do that. | 15:22 |
| gmb | al-maisan: Very impressive branch. r=me with the comment change and one other minor nitpick (see review) | 15:35 |
| al-maisan | gmb: thanks a million! I owe you one :) | 15:35 |
| gmb | :) | 15:35 |
| === matsubara is now known as matsubara-lunch | ||
| === salgado-lunch is now known as salgado | ||
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: jtv, Edwin || reviewing: - || queue [abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === beuno is now known as beuno-lunch | ||
| abentley | EdwinGrubbs: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/no-review-diff/+merge/16900 ? | 16:08 |
| EdwinGrubbs | abentley: sure | 16:09 |
| abentley | EdwinGrubbs: Thanks. | 16:09 |
| abentley | EdwinGrubbs: jtv is listed in the topic, but apparently has connectivity issues. | 16:09 |
| leonardr | flacoste, i'm getting somewhat close to being able to land the first major multiversion branch. i wonder if you're interested in going over it with me. it would be a fairly major project (i have divided it into two chunks but each chunk is larger than 800 lines) | 16:16 |
| leonardr | (ie. i am trying to find someone to do a review, and i thought of you since you're interested in this project) | 16:16 |
| flacoste | leonardr: yes, i'd have time for this this afternoon | 16:17 |
| leonardr | flacoste, ok, great | 16:17 |
| === beuno-lunch is now known as beuno | ||
| EdwinGrubbs | abentley the docstring for test_preview_diff_text_with_no_diff() says that the review_diff should be None when there is no context.preview_diff. Is that right? I don't really understand the difference between a review diff and a preview diff or whether all the objects treat them as separate. | 17:02 |
| abentley | EdwinGrubbs: The confusion between review diffs and preview diffs was one of the reasons we got rid of review diffs. A review diff shows the changes the branch author made. A preview diff shows what would happen if you merged that branch. | 17:03 |
| abentley | EdwinGrubbs: The review_diff field on the view class now actually refers to the preview diff, so the docstring is accurate. | 17:04 |
| EdwinGrubbs | abentley: is that first docstring in the diff correct, or should "review_diff" be "preview_diff". In the test you are checking view.preview_diff_text. | 17:05 |
| abentley | EdwinGrubbs: I think review_diff should be preview_diff_text. | 17:06 |
| EdwinGrubbs | abentley: The docstring for test_preview_diff_utf8 doesn't make any sense. | 17:06 |
| abentley | EdwinGrubbs: It means that if the bytes are utf-8-encoded, they should be decoded to unicode as utf-8. | 17:07 |
| EdwinGrubbs | abentley: can you clarify that in the docstring? | 17:08 |
| abentley | EdwinGrubbs: "A preview_diff in utf-8 should be decoded as utf-8" ? | 17:08 |
| EdwinGrubbs | abentley: yes, but there is no reason to say utf-8 twice, can you say "decoded into a unicode object"? | 17:13 |
| abentley | EdwinGrubbs: The reason to say it twice is because it should not be decoded using a different encoding. | 17:14 |
| abentley | e.g. "A preview_diff in utf-8 should be decoded as utf-8, not utf-16" | 17:14 |
| EdwinGrubbs | abentley: ok, that makes sense, since that's what you are really trying to test. | 17:15 |
| EdwinGrubbs | abentley: in xx-branch-merge-proposals.txt, a tag with id='review-diff' is retrieved. Is that just a case where the UI hasn't yet been updated to match the name of the attribute that is now used? | 17:18 |
| abentley | EdwinGrubbs: Yes. | 17:19 |
| EdwinGrubbs | r=me with the two docstring changes | 17:19 |
| abentley | EdwinGrubbs: Thanks. | 17:19 |
| === matsubara-lunch is now known as matsubara | ||
| === EdwinGrubbs is now known as Edwin-lunch | ||
| leonardr | flacoste, let me know when you're ready to start the review | 17:44 |
| flacoste | send it to me, i'll start on it after lunch | 17:44 |
| leonardr | ok, the first bit is some code i pushed back in november | 17:45 |
| leonardr | https://code.edge.launchpad.net/~leonardr/lazr.restful/version-specific-request-interface/+merge/15172 | 17:45 |
| leonardr | i'm working on writing up the subsequent changes | 17:45 |
| leonardr | i tried to use looms for this branch but i didn't do a very good job | 17:45 |
| leonardr | on top of that branch, i've got one tiny thread and one enormous thread | 17:45 |
| flacoste | leonardr: i'll look at that m-p after lunch and contact you if i have any questions | 17:50 |
| leonardr | ok | 17:56 |
| leonardr | then we'll move from there | 17:56 |
| === flacoste is now known as flacoste_lunch | ||
| === deryck is now known as deryck[lunch] | ||
| === flacoste_lunch is now known as flacoste | ||
| === Edwin-lunch is now known as EdwinGrubbs | ||
| === matsubara is now known as matsubara-afk | ||
| === deryck[lunch] is now known as deryck | ||
| === noodles76 is now known as noodles775 | ||
| flacoste | EdwinGrubbs: can I have r=you for adding a top-level .ctags to the launchpad tree? | 20:07 |
| EdwinGrubbs | flacoste: sure | 20:08 |
| EdwinGrubbs | flacoste: thanks for doing that | 20:08 |
| flacoste | leonardr: around? | 20:27 |
| leonardr | flacoste, yes | 20:27 |
| flacoste | leonardr: do you remember why in multiversion.txt you have to register an IComponentLookup adapter? | 20:27 |
| flacoste | everything_uses_the_global_site_manager | 20:28 |
| leonardr | flacoste: i was modeling it after webservice.txt. i assumed it was because the test was running in a very minimal environment | 20:28 |
| leonardr | i'll try removing it and seeing if it still works | 20:28 |
| leonardr | flacoste, that code is unnecessary both in webservice.txt and multiversion.txt. i'l remove it from both places | 20:29 |
| flacoste | leonardr: ok, thanks! | 20:29 |
| flacoste | leonardr: what is the use of IVersionedClientRequestImplementation | 20:32 |
| flacoste | ? | 20:32 |
| flacoste | I'm reading the "Defining the request interfaces" section | 20:32 |
| flacoste | and the narrative doesn't talk about that interface | 20:33 |
| leonardr | flacoste: IVCRI makes it easy to look up a marker interface based on the version string | 20:33 |
| flacoste | leonardr: ok, you probably want to explain that in the narrative then | 20:33 |
| leonardr | sure | 20:33 |
| flacoste | leonardr: first review done | 20:51 |
| leonardr | flacoste, ok | 20:51 |
| leonardr | because the original branch causes a huge number of test failures (the second branch is all about fixing these), i'm going to make your suggested changes to the second branch | 20:52 |
| flacoste | leonardr: ok, i won't have time to review the second branch today though | 20:53 |
| leonardr | flacoste: no problem | 20:53 |
| leonardr | i'm almost done for the day. i'll spend the rest of the day responding to your review and we can pick up tomorrow | 20:54 |
| leonardr | flacoste: not sure why those conflict markers are showing up | 20:55 |
| leonardr | i'll re-push my branch and hopefully they'll disappear | 20:55 |
| flacoste | leonardr: branch trunk and merge your branch | 20:56 |
| flacoste | see if you get any conflicts | 20:56 |
| leonardr | nope | 21:05 |
| === Ursinha is now known as Ursinha-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
