| rog | morning! | 07:35 |
|---|---|---|
| TheMue | rog: hiya | 08:00 |
| rog | TheMue: yo! | 08:01 |
| === Leseb_ is now known as Leseb | ||
| rog | weird problem: i'm changing a time.Sleep statement. if it's 0.1 seconds, the test runs for 6.749s. if it's 1 second, the test runs for 9.375s. | 08:03 |
| rog | i wonder if time is moving wrongly on this machine | 08:04 |
| rog | ah, easy answer! the test is run three times... | 08:06 |
| TheMue | hehe, so no wonder | 08:09 |
| TheMue | fwereade: morning | 08:35 |
| fwereade | TheMue, heyhey | 08:35 |
| rog | fwereade: i've just submitted the long-time-pending branch of gnuflag that adds SetOutput, amongst other things | 08:44 |
| rog | fwereade: i was waiting for a review from niemeyer, but it never came | 08:44 |
| fwereade | rog, awesome, tyvm | 08:44 |
| fwereade | rog, I have a trivial here to get a very few tests passing again with the new gocheck: http://paste.ubuntu.com/858896/ | 08:45 |
| rog | fwereade: LGTM | 08:46 |
| rog | fwereade: i'm just about to have a look at your new persistence branch BTW | 08:46 |
| rog | fwereade: and thanks for the review of go-ec2-robustness | 08:46 |
| fwereade | rog, a pleasure :) | 08:46 |
| rog | fwereade: glad you liked the attempt type :-) | 08:47 |
| * fwereade tries to remember what branches he has queued up | 08:47 | |
| rog | (i thought it was quite neat) | 08:47 |
| fwereade | rog, yes indeed, that branch made me happy :) | 08:47 |
| rog | fwereade: a previous branch had a version of this code in, which made quite a nice pair with attempt, but it became unnecessary, so i deleted it, with slight regret. http://code.google.com/p/rog-go/source/browse/parallel/parallel.go | 08:49 |
| fwereade | rog, nice :) | 08:50 |
| fwereade | TheMue: I have some trivial changes to make state work with the new gocheck: http://paste.ubuntu.com/858916/ | 09:14 |
| TheMue | fwereade: yep, i'm just doing it | 09:16 |
| fwereade | TheMue, ah sorry, I should have coordinated | 09:16 |
| fwereade | TheMue, rog: is there a part of juju/go whose test failures are *not* being looked at by one of you, that I can pop in and fix quickly? | 09:17 |
| TheMue | fwereade: I'll do everything below state | 09:18 |
| fwereade | TheMue, cool | 09:18 |
| fwereade | rog, is any of what remains under your aegis or shall I just work through them? | 09:19 |
| fwereade | TheMue, do you mean to include store in what you said or not? | 09:19 |
| TheMue | fwereade: no, only state. i don't know what gustavo already did there | 09:20 |
| fwereade | TheMue, rog: ok, I'll pick something not in state at random and go from there | 09:21 |
| TheMue | fwereade: sounds reasonable | 09:21 |
| rog | fwereade: i'm happy to do everything under environs | 09:22 |
| fwereade | TheMue, btw, the patch above seems to fix all the actual failures; it doesn't deal with switching to HasLen where appropriate | 09:22 |
| fwereade | rog, cool | 09:22 |
| rog | fwereade: i'll just go and update gocheck | 09:22 |
| TheMue | fwereade: i'll take a look | 09:22 |
| rog | fwereade: i'm slightly concerned about tests that do: c.Assert(x, Not(Equals), y) | 09:24 |
| rog | fwereade: because they won't necessarily fail though they'll be wrong | 09:24 |
| rog | fwereade: worth looking for, i think | 09:24 |
| fwereade | rog, hmm; I haven't seen many of those thankfully, but I'll try to give every Equals at least a quick glance ;) | 09:25 |
| fwereade | rog, TheMue: would one of you cast an eye over http://paste.ubuntu.com/858930/ please? | 09:35 |
| rog | on it | 09:35 |
| * TheMue too (and I would like a neat diff visualizer) | 09:36 | |
| rog | fwereade: bug := Commentf("ParseURL(%q)", t.s) | 09:36 |
| rog | fwereade: seems wrong | 09:36 |
| rog | fwereade: shouldn't be named "bug" any more... | 09:37 |
| fwereade | rog, good point, ty | 09:37 |
| rog | TheMue: bzr qdiff works well | 09:37 |
| rog | TheMue: also we've got codereview.com :-) | 09:37 |
| TheMue | rog: I meant in web instead of using paste… here | 09:38 |
| rog | TheMue: yeah, well that's why we've got rietveld... | 09:38 |
| fwereade | TheMue, ah, sorry, I was thinking most of these were quite well suited to plain-ol-diff format | 09:39 |
| rog | fwereade: they were | 09:39 |
| rog | fwereade: LGTM modulo s/bug/com(ent)?/ | 09:39 |
| rog | s/ent/ment/ :-) | 09:39 |
| fwereade | rog, cool, I called it comment | 09:39 |
| TheMue | rog, fwereade: like many pastes provide syntax highlighting i just would like a better visualization of a pasted diff. good uis are nothing bad. | 09:40 |
| fwereade | TheMue, rog: do we want double-reviews for these or are we ok with a single LGTM? | 09:41 |
| rog | fwereade: i think we're fine with single LGTMs for these | 09:41 |
| fwereade | TheMue, sorry, I didn't intend any unnecessary cognitive load | 09:42 |
| TheMue | rog, fwereade: ack | 09:42 |
| fwereade | TheMue, I'll lbox propose the next one, doesn't cost me much ;) | 09:42 |
| TheMue | fwereade: no, that's not your problem, no need to excuse. only a feature wish to pasting-plattforms | 09:43 |
| TheMue | fwereade: even w/o lbox it should be possible to create more readable diff vizualizations | 09:44 |
| TheMue | fwereade: at least coloring - in red and + in green | 09:45 |
| fwereade | TheMue, quite so, syntax highlighting on paste.ubuntu.com would be nice (as long as I didn't have to think about it at all ;)) | 09:45 |
| TheMue | fwereade: yep, just paste, the system guesses (or you set a lang or diff) and done | 09:46 |
| fwereade | rog, TheMue: first one to ack gets the glory of reviewing this not-really-very-complex-but-nonetheless-unhighlighted diff: http://paste.ubuntu.com/858944/ | 09:48 |
| rog | ack | 09:48 |
| TheMue | ack | 09:48 |
| rog | fwereade: LGTM | 09:49 |
| rog | fwereade: BTW i think these should probably go through lbox propose anyway, just for the annotations | 09:49 |
| fwereade | rog, I planned to do that, and then cloudinit seemed so trivial as to be mocking me ;) | 09:50 |
| fwereade | rog, will do though | 09:50 |
| rog | fwereade: do 'em all in one branch | 09:50 |
| fwereade | rog, as you like it :) | 09:51 |
| rog | fwereade: (i can't see any reason to do a separate branch for each package - they're all fixes relating to the same thing) | 09:51 |
| fwereade | rog, optimal triviality of reviewing :) | 09:52 |
| rog | amortised anyway... | 09:52 |
| fwereade | rog, or at least "perceived" ;) | 09:53 |
| rog | fwereade: what's a good way to "un-diverge" a branch when it says it's diverged? | 10:00 |
| rog | (i only forked it earlier this morning) | 10:00 |
| rog | fwereade: i did merge with trunk | 10:00 |
| rog | fwereade: still diverged | 10:00 |
| fwereade | rog, not really sure I'm afraid :( | 10:00 |
| fwereade | rog, I have on occasion got myself out of such holes with voodoo and magic that I don't recall | 10:01 |
| rog | ah, i've discovered the problem! | 10:01 |
| fwereade | rog, but last time I think I went straight to diff, fresh branch, patch | 10:01 |
| rog | fwereade: lbox was trying to push to lp:juju/go | 10:01 |
| fwereade | rog, oh yes? | 10:01 |
| rog | fwereade: i forgot to do the manual push | 10:01 |
| fwereade | rog, cool | 10:02 |
| rog | fwereade: would be nice if it mentioned the name of the branch it failed to push to... | 10:02 |
| fwereade | rog, ha, yeah | 10:02 |
| rog | fwereade, TheMue: first to ack gets it: https://codereview.appspot.com/5699085/ | 10:11 |
| fwereade | rog, ack | 10:11 |
| fwereade | rog, TheMue: https://codereview.appspot.com/5701056 | 10:12 |
| rog | fwereade: LGTM | 10:13 |
| fwereade | rog, LGTM also | 10:14 |
| fwereade | rog, get yours in first and I'll merge and submit afterwards | 10:15 |
| rog | fwereade: submitted | 10:15 |
| rog | fwereade: thanks | 10:16 |
| fwereade | rog, and likewise and likewise :) | 10:18 |
| TheMue | So, https://codereview.appspot.com/5671055/ and https://codereview.appspot.com/5690051/ urgently need their LGTM to move into the trunk. | 10:31 |
| rog | TheMue: the second one can't be reviewed until the first one is submitted | 10:32 |
| rog | TheMue: and the first one is waiting for a LGTM from niemeyer, so i can't help, sorry. | 10:33 |
| TheMue | rog: yep, i already asked gustavo for it. | 10:33 |
| rog | TheMue: you'll just have to wait until he's ready, i'm afraid. | 10:33 |
| rog | TheMue: (you can do the gocheck fixes before submitting those branches, of course) | 10:34 |
| TheMue | rog: the last both proposes now contain the gecheck fixes | 10:39 |
| rog | TheMue: you should do the gocheck fixes as a separate propose, i think. | 10:39 |
| rog | TheMue: that way we can get a working state without the necessity to submit both of those branches | 10:40 |
| rog | TheMue: (you can do the fixes as a branch from the current trunk, then merge it into your upcoming branches) | 10:40 |
| TheMue | rog: yeah, could do, but i dislike this juggling. the reviews are really almost ready since now almost 4 days | 10:42 |
| rog | TheMue: yes, i know the feeling very well. it's frustrating, but we have to live with it... | 10:43 |
| TheMue | rog: yep | 10:43 |
| TheMue | lunchtime | 10:43 |
| rog | fwereade: review delivered | 10:46 |
| fwereade | rog, cheers | 10:46 |
| rog | fwereade: trivial matters only... | 10:46 |
| fwereade | rog, sweet :) | 10:46 |
| rog | fwereade: what's with "iNstance" ? | 11:32 |
| fwereade | rog, er, it looks a bit more like an instance id. I totally should have called it "instance-id" | 11:32 |
| * fwereade looks a bit shamefaced | 11:33 | |
| rog | fwereade: i've been using i-aaaaaaaa | 11:33 |
| rog | fwereade: (or similar) | 11:33 |
| rog | (which is actually a valid ec2 instance id) | 11:34 |
| fwereade | rog, yeah, seems smarter | 11:34 |
| rog | fwereade: i thought there was some cross check i hadn't seen | 11:34 |
| TheMue | so, https://codereview.appspot.com/5695073/ is for the current trunk | 11:34 |
| fwereade | rog, no, I'm afraid it does nothing whatsoever | 11:35 |
| === Leseb_ is now known as Leseb | ||
| rog | fwereade: ok, cool. maybe worth changing though | 11:35 |
| fwereade | rog, definitely | 11:35 |
| rog | TheMue: c.Assert(keys, Equals, []string{"m-0", "m-1"}) | 11:36 |
| fwereade | TheMue, I'm 99% sure I spotted some Assert(len(blah), s | 11:37 |
| rog | TheMue: i can't see how this is guaranteed to work | 11:37 |
| rog | oops, sorry | 11:37 |
| rog | TheMue: didn't see the sort.Sot | 11:37 |
| rog | Sort | 11:37 |
| rog | Strings, even | 11:37 |
| TheMue | ;) | 11:37 |
| TheMue | fwereade: tests are green. what problem do you expect? | 11:38 |
| fwereade | TheMue, no failures expected; just that not using HasLen leads to more opaque failures if and when they do happen | 11:39 |
| TheMue | fwereade: oh, will take a look | 11:41 |
| TheMue | rog: what do you mean with "no ItemChange"? | 11:43 |
| rog | TheMue: you could write it as []ItemChange{{....}} | 11:44 |
| rog | er, i think, hold on | 11:44 |
| rog | yeah | 11:44 |
| rog | TheMue: within a slice initialiser, you can omit the type names if they're implied | 11:45 |
| TheMue | rog: ah, yes, one of the g1 changes | 11:45 |
| rog | TheMue: that change has been around for months actually | 11:46 |
| TheMue | rog: yep, missed to use it | 11:46 |
| rog | TheMue: in fact it was in r60 | 11:47 |
| rog | (assuming play.golang.org still uses r60) | 11:47 |
| TheMue | rog: anything else? | 11:47 |
| rog | TheMue: nope | 11:47 |
| rog | just gonna reboot and try and get my system working again | 11:53 |
| TheMue | fwereade, rog: fixed it | 12:00 |
| fwereade | TheMue, LGTM | 12:01 |
| === Leseb_ is now known as Leseb | ||
| rog | niemeyer: hiya | 12:42 |
| niemeyer | rog: Yo | 12:42 |
| rog | niemeyer: i think most of the gocheck changes have been done, BTW | 12:43 |
| rog | niemeyer: just state to be submitted | 12:43 |
| niemeyer | rog: neat, I was just checking them out, thanks for that | 12:43 |
| TheMue | niemeyer: hiya from Germany too | 12:44 |
| niemeyer | TheMue: Hey man | 12:47 |
| niemeyer | fwereade_: You've got a review on the presence node | 14:50 |
| fwereade_ | niemeyer, thanks | 14:50 |
| niemeyer | fwereade_: Let me know if you wanna talk about any of the points | 14:51 |
| niemeyer | fwereade_: The whole thing looks very good | 14:51 |
| fwereade_ | niemeyer, will do :) | 14:51 |
| fwereade_ | niemeyer, sweet :D | 14:51 |
| niemeyer | fwereade_: Mostly details | 14:51 |
| niemeyer | fwereade_: There's a single actual issue in the implementation | 14:51 |
| fwereade_ | niemeyer, bother | 14:51 |
| niemeyer | fwereade_: https://codereview.appspot.com/5672067/diff/16001/state/presence/presence.go#newcode133 | 14:52 |
| niemeyer | fwereade_: I think this is the only one that will require a bit of fiddling | 14:52 |
| fwereade_ | niemeyer, hmm, that was largely inspired by worries of "what if for some reason the remote node starts up with a different timeout?" | 14:53 |
| niemeyer | fwereade_: I don't understand how that changes the point | 14:53 |
| niemeyer | fwereade_: Can you describe further.. ok, what if..? | 14:54 |
| fwereade_ | niemeyer, it's a possible reason to pay the costs you point out | 14:54 |
| fwereade_ | niemeyer, then we might see it "die" because we're expecting it to update more often than it does | 14:54 |
| niemeyer | fwereade_: I mean I'm still missing the problem.. what if it starts with a different timeout.. why do we have to read clock on every ping? | 14:54 |
| fwereade_ | niemeyer, ha, good point, sorry misunderstoof | 14:55 |
| fwereade_ | niemeyer, thanks for that, I'll figure something out :) | 14:56 |
| niemeyer | fwereade_: Thank you | 14:56 |
| niemeyer | fwereade_: Btw, please ignore the first two comments.. they were left-overs from a previous review not delivered | 14:57 |
| fwereade_ | niemeyer, ok | 14:57 |
| rog | fwereade_: wasn't this the same point we discussed before? | 14:57 |
| rog | fwereade_: i thought you were concerned about clock skew | 14:58 |
| fwereade_ | rog, I was, and for some reason that didn't seem relevant just now | 14:58 |
| * fwereade_ tries to switch mental gears properly | 14:59 | |
| fwereade_ | rog, got it, it no longer feels like an issue because the watches are no longer expecting to live through multiple liveness changes | 15:01 |
| rog | fwereade_: actually, i think the real reason it seems ok is that you've got a notion of "now" from the just-changed node... | 15:07 |
| fwereade_ | rog, or alternatively that I know the node has just changed *before* my timeout fired, and so I don't really care when "now" is any more | 15:08 |
| rog | fwereade_: good point | 15:09 |
| TheMue | niemeyer: had a chance to look into https://codereview.appspot.com/5671055/? | 15:15 |
| niemeyer | rog: https://codereview.appspot.com/5698073 is done | 15:34 |
| niemeyer | TheMue: Will look next | 15:34 |
| TheMue | niemeyer: fine, thx | 15:35 |
| rog | niemeyer: that was quick! thanks a lot. | 15:35 |
| niemeyer | rog: np | 15:35 |
| niemeyer | TheMue: done | 15:58 |
| TheMue | niemeyer: ah, fine | 15:59 |
| niemeyer | TheMue: The last comment is wrong.. the one about s/anyone/it/ | 15:59 |
| niemeyer | TheMue: It should be "any other charm" rather than "anyone" | 15:59 |
| TheMue | niemeyer: ok, will take care | 15:59 |
| niemeyer | TheMue: Cheers | 15:59 |
| TheMue | niemeyer: regarding the bundle url, shall it be a charm.URL too? so far it has been just a string | 16:05 |
| niemeyer | TheMue: No, that's not a charm URL.. it's a real URL | 16:06 |
| niemeyer | TheMue: http, https, whtaever | 16:07 |
| TheMue | niemeyer: got it, will take net/url.URL | 16:08 |
| niemeyer | TheMue: That'd be fine, yeah | 16:08 |
| rog | niemeyer: changes made. you might want to take a brief look before i submit as i've made the gocheck changes too. | 16:12 |
| niemeyer | rog: Nice, thanks | 16:15 |
| niemeyer | rog: LGTM | 16:15 |
| rog | niemeyer: thanks | 16:15 |
| * niemeyer => lunch! | 16:16 | |
| niemeyer | rog: np | 16:16 |
| rog | niemeyer: enjoy | 16:16 |
| rog | niemeyer: (submitted BTW) | 16:16 |
| rog | fwereade_, niemeyer: this is my current test for zk concurrent close. it's fairly complex and not a little fragile. i'm thinking perhaps i should chuck it in and do something that does a much more simple verification of a single non-zk-read-lock-taking operation, because the read lock code is so simple to verify by eye. what do you think? | 16:33 |
| rog | http://paste.ubuntu.com/859386/ | 16:33 |
| rog | fwereade_: (you might recognise your proxy code, in heavily mutated form...) | 16:34 |
| fwereade_ | rog, heh; when you say "fragile" do you mean "sometimes breaks in practice", or just "assumes more than it should to be a nice black box test"? | 16:36 |
| rog | fwereade_: i mean "might break in practice on a heavily loaded machine" | 16:36 |
| rog | fwereade_: because it has to do a sleep to ensure that the right piece of code takes the lock at the right time | 16:36 |
| rog | fwereade_: there's no guarantee that it works | 16:36 |
| fwereade_ | rog, ah, I missed that, hmm | 16:36 |
| rog | fwereade_: of course 1/20th of a second *should* be perfectly sufficient, but... | 16:37 |
| fwereade_ | rog, indeed | 16:37 |
| rog | fwereade_: i can't think of another way to do it, as we need to test the two operations concurrently and there's no way to "get inside" either of them | 16:37 |
| rog | fwereade_: unless we just do that and have an operation (in export_test.go) whose raison d'etre is to test the locking logic. | 16:38 |
| rog | fwereade_: but that can't be used to directly test all the operations. | 16:38 |
| fwereade_ | rog, I am inclined to go with the "50ms should be enough for anybody" approach, but expand please: what would you put in export_test to get around this? | 16:39 |
| rog | fwereade_: i'd add a fake test operation to zk.Conn which took the read lock, then waited for notification, then relinquished it. | 16:39 |
| rog | fwereade_: so then at least we could test the Close behaviour | 16:40 |
| rog | fwereade_: and all the operations are tested "by analogy". not great. but can you think of a better way? | 16:40 |
| fwereade_ | rog, afraid not | 16:41 |
| fwereade_ | rog, I think I'd prefer theoretically-unsound tests of the actual behaviour | 16:41 |
| rog | fwereade_: it seems like a lot of code to test something which is really quite simple. (as you saw it only took about 5 minutes to implement, and it's quite easy to verify by hand) | 16:41 |
| rog | fwereade_: and it can't test all of the operations either | 16:42 |
| rog | fwereade_: (it has to omit the ones that don't interact with the zk server) | 16:42 |
| fwereade_ | rog, if I think of anything better I'll let you know ;) | 16:44 |
| fwereade_ | rog, afk a sec | 16:44 |
| rog | fwereade_: afk? | 16:44 |
| jimbaker | rog, away from keyboard | 16:46 |
| rog | jimbaker: ah, thanks. hadn't seen that one before. | 16:46 |
| rog | jimbaker: good morning, BTW! | 16:46 |
| jimbaker | rog, thanks! and hopefully a nice evening to you! :) | 16:47 |
| rog | jimbaker: more like afternoon. evening approaches though. | 16:47 |
| rog | jimbaker: pretty grey day, to be honest. | 16:47 |
| jimbaker | rog, my impressions of newcastle, formed purely by literature, are always of a gray day, lashed by the winds of the north sea :) | 16:49 |
| jimbaker | this may or may not have a basis in truth | 16:50 |
| rog | jimbaker: well, north sea about 20 east of here, but it can be gray. although it can be beautifully sunny too! | 16:50 |
| rog | s/20/20 miles/ | 16:50 |
| jimbaker | rog, glad to hear that. probably my mind is fixated on wind, we just went through about 5 days of strong winds here. at my house, gusts of approx 150 km/hr. enough to see grassfires, trucks lying on their sides, bicyclists blown over... now that's some imagery | 16:53 |
| rog | jimbaker: that is strong. we've had some fairly good winds here this season (our house faces the prevailing westerlies) but nothing like that | 16:54 |
| jimbaker | rog, same with us in terms of exposure to westerlies, which are the strongest winds here. anyway, it's the worst in the 11 years i have lived here | 16:56 |
| jimbaker | but it's back to calm and sunny skies. the mountains made us aware of their presence, they are now back to quiet brooding | 16:57 |
| TheMue | niemeyer: https://codereview.appspot.com/5671055/ is in again | 17:00 |
| niemeyer | TheMue: cheers! | 17:30 |
| niemeyer | TheMue: TestCharm still needs dropping | 17:35 |
| niemeyer | TheMue: It's fully redundant with the test followin git | 17:35 |
| niemeyer | TheMue: Otherwise, LGTM | 17:35 |
| TheMue | niemeyer: oh, did i missed it? shit. | 17:37 |
| TheMue | niemeyer: ok, will propose it in a few minutes again. btw, rog reminded me that i can remove ItemChange inside of []ItemChange{}. Could do it quickly too. | 17:38 |
| rog | TheMue: why not leave that for the next one? | 17:38 |
| TheMue | rog: could do so too. only because it's a quick one. | 17:39 |
| rog | TheMue: your call | 17:39 |
| TheMue | niemeyer: what do you say? | 17:40 |
| niemeyer | TheMue: Works either way for me, as long as that's the end of it | 17:43 |
| === TheMue_ is now known as TheMue | ||
| TheMue | Gna! Connection lost. | 17:48 |
| TheMue | niemeyer: so, hopefully last try | 17:58 |
| rog | pwd | 18:00 |
| rog | oops | 18:00 |
| niemeyer | TheMue: TestCharm is *still* there!? | 18:04 |
| rog | i'm off for the day, see y'all tomorrow | 18:05 |
| niemeyer | rog: Have a good one | 18:05 |
| TheMue | niemeyer: arg, sorry, got you wrong. though you meant the url comparison and the last assert should be removed, not the whole test. *sigh* | 18:06 |
| TheMue | niemeyer: my fault. | 18:06 |
| niemeyer | TheMue: All of the lines in TestCharm may be found in that precise shape in the test following it | 18:06 |
| niemeyer | TheMue: So there's no point in having it | 18:07 |
| TheMue | niemeyer: yep, i understand, only understood your review wrong | 18:07 |
| TheMue | niemeyer: i have been too much focused on the url | 18:08 |
| niemeyer | TheMue: Cool, np | 18:08 |
| TheMue | niemeyer: so one last propose ... | 18:09 |
| niemeyer | TheMue: Please just drop the test and submit | 18:09 |
| TheMue | niemeyer: ok, that's shorter ;) | 18:09 |
| TheMue | niemeyer: sadly you stole me some test methods today, so my high watermark is lower now :( | 18:10 |
| TheMue | niemeyer: i'm still not secure with lbox submit, i updated my trunk, merged it into my change and now wanted to submit it. but it says the branch is not clean. do i have to do a commit before lbox submit? | 18:23 |
| niemeyer | TheMue: If you have changes in your branch, you necessarily must commit them, or revert if you don't want them in the final merge | 18:23 |
| TheMue | niemeyer: i committed my changes before the merge (it's the removing of AddCharm() ) | 18:24 |
| niemeyer | TheMue: submit will deliver the changes that you did in your branch onto the target branch.. it will not do changes behind your back | 18:24 |
| niemeyer | TheMue: So why is the branch dirty? | 18:24 |
| TheMue | niemeyer: after my commit i've done a bzr pull in my go-trunk (the base of this propose), then i changed back to my go-state-… and do a bzr merge ../go-trunk there | 18:26 |
| TheMue | niemeyer: here i got no conflicts and a final go test in state worked fine | 18:28 |
| niemeyer | TheMue: Sure.. and then you have to commit that.. the merge will not be committed automatically | 18:29 |
| TheMue | niemeyer: ah, ok, that's missing. lbox help also states a commit, so i thought it's done automatically. | 18:30 |
| niemeyer | TheMue: It will commit the merge of your branch onto the target branch | 18:31 |
| niemeyer | TheMue: It will not fiddle with unfinished states in your own branch | 18:31 |
| TheMue | niemeyer: still not absolutely safe in the world of dvcs. | 18:34 |
| TheMue | niemeyer: thx for your help | 18:34 |
| TheMue | niemeyer: it's in | 18:39 |
| niemeyer | TheMue: Woohay! | 18:39 |
| hazmat | niemeyer, does lbox -cr support prerequisite branches? | 19:12 |
| jimbaker | hazmat, use the -req option | 19:15 |
| hazmat | jimbaker, thanks | 19:17 |
| niemeyer | hazmat: Kind of | 19:19 |
| niemeyer | hazmat: THis is my first TODO after the store work | 19:19 |
| niemeyer | hazmat: It won't diff the branches properly, so all but the first branch in the pipeline will contain data for the previous entries too | 19:19 |
| niemeyer | hazmat: So, in practice, it doesn't work very well yet | 19:20 |
| niemeyer | hazmat: I didn't want to fix that before submit was working, though, to prevent people mistakingly submitting follow up branches thinking that this was _just_ what they were changing | 19:20 |
| hazmat | niemeyer, thanks for clarifying, i was noticing that problem but wasn't sure if -req was obmitted or would help. | 19:20 |
| niemeyer | hazmat: Now submit works, so I'll make submitting check to see if the pre-req is in | 19:20 |
| niemeyer | hazmat: and will make propose diff against the previous branch in the pipeline | 19:21 |
| niemeyer | hazmat: Will work on this as soon as store is out of the way | 19:21 |
| hazmat | niemeyer, sounds good | 19:21 |
| niemeyer | andrewsmedina: How're things going there? | 19:22 |
| andrewsmedina | niemeyer: hi | 19:24 |
| niemeyer | andrewsmedina: Heya | 19:24 |
| andrewsmedina | niemeyer: I started the local provider | 19:25 |
| niemeyer | andrewsmedina: Sweet! | 19:25 |
| andrewsmedina | niemeyer: inspired by ec2 provider | 19:25 |
| niemeyer | andrewsmedina: Super | 19:25 |
| andrewsmedina | niemeyer: but I had some problems with gocheck | 19:25 |
| niemeyer | andrewsmedina: Hmmm.. when did you do that? | 19:25 |
| hazmat | bcsaller1, is the subordinate spec ready for niemeyer to have another look? | 19:26 |
| niemeyer | andrewsmedina: Do you want an introduction to the development process via a low-hanging fruit? | 19:26 |
| bcsaller1 | hazmat: yeah, thought it was in the queue, I'll check again | 19:27 |
| andrewsmedina | niemeyer: low-hanging fruit? | 19:27 |
| hazmat | bcsaller1, it was proposed again to reitveld afaics | 19:27 |
| niemeyer | bcsaller1: Maybe it was and I just overlooked it.. please let me know of the URL and I'll check it out | 19:27 |
| hazmat | er. wasn't | 19:27 |
| niemeyer | andrewsmedina: A simpler change that allows to see how things work | 19:28 |
| niemeyer | andrewsmedina: It's probably better than doing a lot of work without touching base as your first task | 19:28 |
| andrewsmedina | niemeyer: It would be amazing :D | 19:28 |
| hazmat | bcsaller1, it looks like it was just pushed again to lp, lbox propose again should do the trick | 19:28 |
| niemeyer | andrewsmedina: This is a good candidate: https://codereview.appspot.com/5532098/diff/4001/juju/charm/metadata.py | 19:28 |
| bcsaller1 | niemeyer, hazmat: it doesn't appear to upload the new push, I'll try it now | 19:29 |
| bcsaller1 | must have gotten an error before I didn't see | 19:29 |
| niemeyer | bcsaller1, hazmat: Was that original spec merged already? | 19:29 |
| bcsaller1 | no | 19:29 |
| niemeyer | bcsaller1: Ouch | 19:29 |
| niemeyer | bcsaller1: What happened? | 19:29 |
| bcsaller1 | kapil had to +1 it as well and then there were the changes you'll be looking at now | 19:29 |
| andrewsmedina | niemeyer: what needs to be done? | 19:30 |
| niemeyer | andrewsmedina: We need to port that change onto the Go side | 19:30 |
| niemeyer | andrewsmedina: Should be a pretty comfortable ride | 19:30 |
| andrewsmedina | ok | 19:30 |
| niemeyer | andrewsmedina: This will affect charm/meta.go | 19:30 |
| niemeyer | andrewsmedina: and the respective tests, of course | 19:30 |
| andrewsmedina | niemeyer: how you set your dev enviroment and run the tests? | 19:30 |
| niemeyer | andrewsmedina: I do: export GOPATH=$HOME | 19:31 |
| andrewsmedina | niemeyer: I did checkout in gopath | 19:31 |
| niemeyer | andrewsmedina: and "go get launchpad.net/juju/go/charm", for example | 19:31 |
| andrewsmedina | niemeyer: and I run, tests module by module | 19:31 |
| niemeyer | andrewsmedina: This will set up all dependencies | 19:31 |
| andrewsmedina | niemeyer: yes | 19:31 |
| niemeyer | bcsaller1: :-( | 19:31 |
| niemeyer | bcsaller1: We spent a lot of energy to agree on that state | 19:32 |
| andrewsmedina | niemeyer I'm on the right way | 19:32 |
| bcsaller1 | niemeyer: we talked about this at the call on Thursday | 19:32 |
| niemeyer | bcsaller1: If Kapil had comments on it, they should be applied on the original state.. now we have an unrelated new change that is piled up together with the whole spec, and we've got no agreement committed yet | 19:33 |
| bcsaller1 | niemeyer: I saw them as related changes from the needed second +1 | 19:34 |
| niemeyer | bcsaller1: I know, but I thougth you were talking about a _new_ branch.. I had no idea that this change that we agreed on is still floating around uncommitted | 19:34 |
| bcsaller1 | I have the full revision history. This is getting merged into the docs branch anyway, we can build whatever history you want there | 19:35 |
| niemeyer | bcsaller1: Do you have the link at hand? | 19:35 |
| bcsaller1 | niemeyer: https://codereview.appspot.com/5541054/ | 19:36 |
| niemeyer | bcsaller1: All I see in that link is "Thanks a lot for the extensive spec, and for keeping up with it Ben. Just a few | 19:36 |
| niemeyer | last minute trivials." | 19:36 |
| niemeyer | bcsaller1: and hten "Please take another look." | 19:36 |
| niemeyer | bcsaller1: Where are the comments from Kapil that you are addressing? | 19:36 |
| bcsaller1 | niemeyer: prior G+ talks , there are the changes brought up in the last meeting reflected in subordinate-internals | 19:37 |
| niemeyer | This is really bad.. | 19:37 |
| niemeyer | This is a spec from several months ago.. that was approved more than 2 weeks ago, and now it revives. | 19:38 |
| * niemeyer looks.. | 19:38 | |
| niemeyer | bcsaller1, hazmat: | 19:44 |
| niemeyer | 49 global/ | 19:44 |
| niemeyer | 50 global/ | 19:44 |
| niemeyer | !? | 19:44 |
| bcsaller1 | niemeyer: thats so that the path structure is the same for all types | 19:45 |
| bcsaller1 | container/unit-0001/... or global/global/... | 19:45 |
| bcsaller1 | the same path construction logic can then be used | 19:45 |
| niemeyer | bcsaller1: Sorry, I don't get.. | 19:46 |
| hazmat | that feels a little strange | 19:46 |
| hazmat | also for containers the settings node should be under the container | 19:46 |
| bcsaller1 | one is relation_scope_type/relation_scope/value | 19:46 |
| bcsaller1 | err... one is relation_scope_type/relation_scope_value | 19:46 |
| hazmat | bcsaller1, but its redundant | 19:47 |
| hazmat | bcsaller1, global should suffice for scope and value | 19:47 |
| bcsaller1 | hazmat: not really, it looks that way to a human, but its a uniform structure to the apis | 19:48 |
| hazmat | bcsaller1, the api can interpret global scope however it chooses | 19:48 |
| hazmat | bcsaller1, and settings should always be under scope, else the additional containment serves little purpose imo.. effectively this is reuse of the existing relation work/watchers, with a prefix to the base path | 19:49 |
| bcsaller1 | hazmat: agreed, I guess I didn't juggle it properly | 19:50 |
| niemeyer | bcsaller1: I don't understand.. you have to special case it anyway to pick global.. in fact why don't we even have any "global" in there at all? | 19:50 |
| niemeyer | s/don't we/do we/ | 19:50 |
| niemeyer | bcsaller1: I assume we can tell whether a relation is a container relation or not based on the topology information, right? | 19:51 |
| bcsaller1 | niemeyer: it mirrors container/unit-0001, container/unit-00002 etc. the path is relations/relation_id/relation_scope_type/relation_scope_value/relation_role/... where things under relation_scope_value are the previous structure | 19:51 |
| bcsaller1 | niemeyer: yes, that has to be the case | 19:51 |
| niemeyer | bcsaller1: So why do we even have "container" or "global" nodes? | 19:52 |
| niemeyer | bcsaller1: What problem are these solving? | 19:52 |
| niemeyer | bcsaller1: In fact, what problem is the change from the original spec that was last reviewing solving? | 19:53 |
| niemeyer | last reviewed, sorry.. I can't write anymore | 19:53 |
| bcsaller1 | niemeyer: I think it helps with a number of things. It helps address the case of principal service A with subs B and C, where B/0 and C/0 have a relation contained by A/0 | 19:54 |
| niemeyer | bcsaller1: Ok, I guess it enables watching more specifically.. | 19:54 |
| bcsaller1 | that as well | 19:54 |
| niemeyer | bcsaller1: as well? What else? | 19:54 |
| bcsaller1 | it was very hard to understand container relations between services where neither of them was the container before | 19:55 |
| niemeyer | bcsaller1: How is that related to the structure of the nodes? | 19:57 |
| hazmat | it solves having to do some dereferencing when establishing what sort of watch to establish, else the filtering has to do quite a few lookups to establish a filter, it also effectively preserves reuse of the existing watch logic, just with a prefix path for existing relation types.. ie. effectively it encodes the additional axis of scope as part of the physical structure | 19:58 |
| bcsaller1 | hazmat: thanks, yes | 19:59 |
| hazmat | ugh.. too much 'effectively'.. is that like a double negative ;-) | 19:59 |
| niemeyer | hazmat: I see, cheers | 19:59 |
| hazmat | bcsaller1, even given that.. the global/global can just be 'global', and settings should be moved under the scope | 20:00 |
| niemeyer | bcsaller1: So just s,global/global/,global/, please | 20:00 |
| bcsaller1 | I can but it makes the handling code non-uniform, I don't know that its really better but I'm willing to add branch code in there | 20:01 |
| niemeyer | bcsaller1, hazmat: Hmm.. | 20:01 |
| niemeyer | bcsaller1, hazmat: Can an individual principal have more than one subordinate under the same relation? | 20:03 |
| niemeyer | bcsaller1, hazmat: And vice-versa? | 20:03 |
| bcsaller1 | container/unit-id/ will exist once for each container in the relationship | 20:04 |
| bcsaller1 | subordinates can still only have once principal | 20:04 |
| hazmat | niemeyer, no.. they'd be different relations, relations are pair-wise service 2 service | 20:04 |
| niemeyer | hazmat: How many units may a given relation have? | 20:05 |
| hazmat | niemeyer, arbitrary | 20:05 |
| hazmat | er. infinite | 20:05 |
| bcsaller1 | limited by ZK memory ;) | 20:06 |
| niemeyer | hazmat: how might that happen when we're talking about container relations? | 20:06 |
| hazmat | niemeyer, it wont .. for container relations, its basically pair wise on unit 2 unit basis | 20:06 |
| niemeyer | hazmat: So you're saying that a given relation rel-000000042, will only ever have two units? | 20:07 |
| hazmat | niemeyer, no.. it will have n units.. where n is the number of primary/principal service units | 20:07 |
| hazmat | er.. 2n | 20:07 |
| hazmat | with the additional count coming from subordinates | 20:08 |
| hazmat | but a given container scope for that relation will have two units | 20:08 |
| niemeyer | hazmat: Aha, ok | 20:08 |
| niemeyer | hazmat: That structure still feels a bit overblown | 20:09 |
| niemeyer | hazmat: Have you had a chance to see the branch that has the code reuse etc? | 20:10 |
| bcsaller1 | niemeyer: he hasn't | 20:10 |
| hazmat | niemeyer, i have not, but i can envision it fairly easily | 20:10 |
| niemeyer | hazmat: Ok.. why do you want to move settings again? | 20:11 |
| hazmat | niemeyer, two reasons, it preserves the watch semantics with the use of the prefix, and it keeps the information about scope within the context of that scope. | 20:13 |
| niemeyer | hazmat: Sounds good | 20:13 |
| hazmat | cool | 20:13 |
| niemeyer | hazmat: This means we can take both "global" keys out | 20:14 |
| niemeyer | hazmat: and that we can take the "container" keyword out as well | 20:14 |
| niemeyer | hazmat: So that relation-0000N has either: | 20:14 |
| niemeyer | 1) A list of unit names, when it's a container relation | 20:14 |
| niemeyer | or | 20:14 |
| niemeyer | 2) server, client, settings | 20:14 |
| niemeyer | The latter, (2), happens to be exactly what one would find inside the unit name of (1). | 20:15 |
| niemeyer | So global/global/, and container/, can both go away | 20:15 |
| niemeyer | hazmat: WDYT? | 20:15 |
| hazmat | niemeyer, yeah.. i had hoped for that earlier as well during a g+ discussion.. i'm fine witht hat, bcsaller1 had raised a question regarding that still leaving a question of where to find the scope, but i still think that comes out/falls out from retrieving the unit relation state | 20:17 |
| hazmat | iotw, works for me | 20:17 |
| hazmat | bcsaller1, what do you think? | 20:17 |
| niemeyer | hazmat: The scope comes out of the topology, IMO | 20:18 |
| niemeyer | hazmat: No matter what, the implementation shouldn't be _listing_ relation-0000N to know what to do | 20:18 |
| hazmat | niemeyer, agreed, topology is probably the right place | 20:19 |
| bcsaller1 | I'm fine with that as well | 20:19 |
| niemeyer | hazmat: and it should already be there anyway right now.. | 20:19 |
| niemeyer | bcsaller1: Super | 20:19 |
| hazmat | cool, sounds like agreement | 20:19 |
| hazmat | let's ship it ;-) | 20:19 |
| bcsaller1 | I'll make some revisions and push it again :-/ | 20:19 |
| === Leseb_ is now known as Leseb | ||
| niemeyer | bcsaller1: Sent these notes onto the review | 20:24 |
| bcsaller1 | niemeyer: thanks | 20:24 |
| niemeyer | bcsaller1: np | 20:30 |
| niemeyer | I'm stepping out for some exercising outside.. will be back later today | 20:31 |
| === bcsaller1 is now known as bcsaller | ||
| hazmat | i'm gonna do the same, bbiab | 20:33 |
| * hazmat yawns | 21:29 | |
| === Leseb_ is now known as Leseb | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!