TheMuerog: hiya08:00
rogTheMue: yo!08:01
=== Leseb_ is now known as Leseb
rogweird 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
rogi wonder if time is moving wrongly on this machine08:04
rogah, easy answer! the test is run three times...08:06
TheMuehehe, so no wonder08:09
TheMuefwereade: morning08:35
fwereadeTheMue, heyhey08:35
rogfwereade: i've just submitted the long-time-pending branch of gnuflag that adds SetOutput, amongst other things08:44
rogfwereade: i was waiting for a review from niemeyer, but it never came08:44
fwereaderog, awesome, tyvm08:44
fwereaderog, I have a trivial here to get a very few tests passing again with the new gocheck: http://paste.ubuntu.com/858896/08:45
rogfwereade: LGTM08:46
rogfwereade: i'm just about to have a look at your new persistence branch BTW08:46
rogfwereade: and thanks for the review of go-ec2-robustness08:46
fwereaderog, a pleasure :)08:46
rogfwereade: glad you liked the attempt type :-)08:47
* fwereade tries to remember what branches he has queued up08:47
rog(i thought it was quite neat)08:47
fwereaderog, yes indeed, that branch made me happy :)08:47
rogfwereade: 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.go08:49
fwereaderog, nice :)08:50
fwereadeTheMue: I have some trivial changes to make state work with the new gocheck: http://paste.ubuntu.com/858916/09:14
TheMuefwereade: yep, i'm just doing it09:16
fwereadeTheMue, ah sorry, I should have coordinated09:16
fwereadeTheMue, 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
TheMuefwereade: I'll do everything below state09:18
fwereadeTheMue, cool09:18
fwereaderog, is any of what remains under your aegis or shall I just work through them?09:19
fwereadeTheMue, do you mean to include store in what you said or not?09:19
TheMuefwereade: no, only state. i don't know what gustavo already did there09:20
fwereadeTheMue, rog: ok, I'll pick something not in state at random and go from there09:21
TheMuefwereade: sounds reasonable09:21
rogfwereade: i'm happy to do everything under environs09:22
fwereadeTheMue, btw, the patch above seems to fix all the actual failures; it doesn't deal with switching to HasLen where appropriate09:22
fwereaderog, cool09:22
rogfwereade: i'll just go and update gocheck09:22
TheMuefwereade: i'll take a look09:22
rogfwereade: i'm slightly concerned about tests that do: c.Assert(x, Not(Equals), y)09:24
rogfwereade: because they won't necessarily fail though they'll be wrong09:24
rogfwereade: worth looking for, i think09:24
fwereaderog, hmm; I haven't seen many of those thankfully, but I'll try to give every Equals at least a quick glance ;)09:25
fwereaderog, TheMue: would one of you cast an eye over http://paste.ubuntu.com/858930/ please?09:35
rogon it09:35
* TheMue too (and I would like a neat diff visualizer)09:36
rogfwereade: bug := Commentf("ParseURL(%q)", t.s)09:36
rogfwereade: seems wrong09:36
rogfwereade: shouldn't be named "bug" any more...09:37
fwereaderog, good point, ty09:37
rogTheMue: bzr qdiff works well09:37
rogTheMue: also we've got codereview.com :-)09:37
TheMuerog: I meant in web instead of using paste… here09:38
rogTheMue: yeah, well that's why we've got rietveld...09:38
fwereadeTheMue, ah, sorry, I was thinking most of these were quite well suited to plain-ol-diff format09:39
rogfwereade: they were09:39
rogfwereade: LGTM modulo s/bug/com(ent)?/09:39
rogs/ent/ment/ :-)09:39
fwereaderog, cool, I called it comment09:39
TheMuerog, 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
fwereadeTheMue, rog: do we want double-reviews for these or are we ok with a single LGTM?09:41
rogfwereade: i think we're fine with single LGTMs for these09:41
fwereadeTheMue, sorry, I didn't intend any unnecessary cognitive load09:42
TheMuerog, fwereade: ack09:42
fwereadeTheMue, I'll lbox propose the next one, doesn't cost me much ;)09:42
TheMuefwereade: no, that's not your problem, no need to excuse. only a feature wish to pasting-plattforms09:43
TheMuefwereade: even w/o lbox it should be possible to create more readable diff vizualizations09:44
TheMuefwereade: at least coloring - in red and + in green09:45
fwereadeTheMue, 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
TheMuefwereade: yep, just paste, the system guesses (or you set a lang or diff) and done09:46
fwereaderog, 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
rogfwereade: LGTM09:49
rogfwereade: BTW i think these should probably go through lbox propose anyway, just for the annotations09:49
fwereaderog, I planned to do that, and then cloudinit seemed so trivial as to be mocking me ;)09:50
fwereaderog, will do though09:50
rogfwereade: do 'em all in one branch09:50
fwereaderog, as you like it :)09:51
rogfwereade: (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
fwereaderog, optimal triviality of reviewing :)09:52
rogamortised anyway...09:52
fwereaderog, or at least "perceived" ;)09:53
rogfwereade: 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
rogfwereade: i did merge with trunk10:00
rogfwereade: still diverged10:00
fwereaderog, not really sure I'm afraid :(10:00
fwereaderog, I have on occasion got myself out of such holes with voodoo and magic that I don't recall10:01
rogah, i've discovered the problem!10:01
fwereaderog, but last time I think I went straight to diff, fresh branch, patch10:01
rogfwereade: lbox was trying to push to lp:juju/go10:01
fwereaderog, oh yes?10:01
rogfwereade: i forgot to do the manual push10:01
fwereaderog, cool10:02
rogfwereade: would be nice if it mentioned the name of the branch it failed to push to...10:02
fwereaderog, ha, yeah10:02
rogfwereade, TheMue: first to ack gets it: https://codereview.appspot.com/5699085/10:11
fwereaderog, ack10:11
fwereaderog, TheMue: https://codereview.appspot.com/570105610:12
rogfwereade: LGTM10:13
fwereaderog, LGTM also10:14
fwereaderog, get yours in first and I'll merge and submit afterwards10:15
rogfwereade: submitted10:15
rogfwereade: thanks10:16
fwereaderog, and likewise and likewise :)10:18
TheMueSo, https://codereview.appspot.com/5671055/ and https://codereview.appspot.com/5690051/ urgently need their LGTM to move into the trunk.10:31
rogTheMue: the second one can't be reviewed until the first one is submitted10:32
rogTheMue: and the first one is waiting for a LGTM from niemeyer, so i can't help, sorry.10:33
TheMuerog: yep, i already asked gustavo for it.10:33
rogTheMue: you'll just have to wait until he's ready, i'm afraid.10:33
rogTheMue: (you can do the gocheck fixes before submitting those branches, of course)10:34
TheMuerog: the last both proposes now contain the gecheck fixes10:39
rogTheMue: you should do the gocheck fixes as a separate propose, i think.10:39
rogTheMue: that way we can get a working state without the necessity to submit both of those branches10:40
rogTheMue: (you can do the fixes as a branch from the current trunk, then merge it into your upcoming branches)10:40
TheMuerog: yeah, could do, but i dislike this juggling. the reviews are really almost ready since now almost 4 days10:42
rogTheMue: yes, i know the feeling very well. it's frustrating, but we have to live with it...10:43
TheMuerog: yep10:43
rogfwereade: review delivered10:46
fwereaderog, cheers10:46
rogfwereade: trivial matters only...10:46
fwereaderog, sweet :)10:46
rogfwereade: what's with "iNstance" ?11:32
fwereaderog, er, it looks a bit more like an instance id. I totally should have called it "instance-id"11:32
* fwereade looks a bit shamefaced11:33
rogfwereade: i've been using i-aaaaaaaa11:33
rogfwereade: (or similar)11:33
rog(which is actually a valid ec2 instance id)11:34
fwereaderog, yeah, seems smarter11:34
rogfwereade: i thought there was some cross check i hadn't seen11:34
TheMueso, https://codereview.appspot.com/5695073/ is for the current trunk11:34
fwereaderog, no, I'm afraid it does nothing whatsoever11:35
=== Leseb_ is now known as Leseb
rogfwereade: ok, cool. maybe worth changing though11:35
fwereaderog, definitely11:35
rogTheMue:        c.Assert(keys, Equals, []string{"m-0", "m-1"})11:36
fwereadeTheMue, I'm 99% sure I spotted some Assert(len(blah), s11:37
rogTheMue: i can't see how this is guaranteed to work11:37
rogoops, sorry11:37
rogTheMue: didn't see the sort.Sot11:37
rogStrings, even11:37
TheMuefwereade: tests are green. what problem do you expect?11:38
fwereadeTheMue, no failures expected; just that not using HasLen leads to more opaque failures if and when they do happen11:39
TheMuefwereade: oh, will take a look11:41
TheMuerog: what do you mean with "no ItemChange"?11:43
rogTheMue: you could write it as []ItemChange{{....}}11:44
roger, i think, hold on11:44
rogTheMue: within a slice initialiser, you can omit the type names if they're implied11:45
TheMuerog: ah, yes, one of the g1 changes11:45
rogTheMue: that change has been around for months actually11:46
TheMuerog: yep, missed to use it11:46
rogTheMue: in fact it was in r6011:47
rog(assuming play.golang.org still uses r60)11:47
TheMuerog: anything else?11:47
rogTheMue: nope11:47
rogjust gonna reboot and try and get my system working again11:53
TheMuefwereade, rog: fixed it12:00
fwereadeTheMue, LGTM12:01
=== Leseb_ is now known as Leseb
rogniemeyer: hiya12:42
niemeyerrog: Yo12:42
rogniemeyer: i think most of the gocheck changes have been done, BTW12:43
rogniemeyer: just state to be submitted12:43
niemeyerrog: neat, I was just checking them out, thanks for that12:43
TheMueniemeyer: hiya from Germany too12:44
niemeyerTheMue: Hey man12:47
niemeyerfwereade_: You've got a review on the presence node14:50
fwereade_niemeyer, thanks14:50
niemeyerfwereade_: Let me know if you wanna talk about any of the points14:51
niemeyerfwereade_: The whole thing looks very good14:51
fwereade_niemeyer, will do :)14:51
fwereade_niemeyer, sweet :D14:51
niemeyerfwereade_: Mostly details14:51
niemeyerfwereade_: There's a single actual issue in the implementation14:51
fwereade_niemeyer, bother14:51
niemeyerfwereade_: https://codereview.appspot.com/5672067/diff/16001/state/presence/presence.go#newcode13314:52
niemeyerfwereade_: I think this is the only one that will require a bit of fiddling14: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
niemeyerfwereade_: I don't understand how that changes the point14:53
niemeyerfwereade_: Can you describe further.. ok, what if..?14:54
fwereade_niemeyer, it's a possible reason to pay the costs you point out14:54
fwereade_niemeyer, then we might see it "die" because we're expecting it to update more often than it does14:54
niemeyerfwereade_: 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 misunderstoof14:55
fwereade_niemeyer, thanks for that, I'll figure something out :)14:56
niemeyerfwereade_: Thank you14:56
niemeyerfwereade_: Btw, please ignore the first two comments.. they were left-overs from a previous review not delivered14:57
fwereade_niemeyer, ok14:57
rogfwereade_: wasn't this the same point we discussed before?14:57
rogfwereade_: i thought you were concerned about clock skew14:58
fwereade_rog, I was, and for some reason that didn't seem relevant just now14:58
* fwereade_ tries to switch mental gears properly14:59
fwereade_rog, got it, it no longer feels like an issue because the watches are no longer expecting to live through multiple liveness changes15:01
rogfwereade_: 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 more15:08
rogfwereade_: good point15:09
TheMueniemeyer: had a chance to look into https://codereview.appspot.com/5671055/?15:15
niemeyerrog: https://codereview.appspot.com/5698073 is done15:34
niemeyerTheMue: Will look next15:34
TheMueniemeyer: fine, thx15:35
rogniemeyer: that was quick! thanks a lot.15:35
niemeyerrog: np15:35
niemeyerTheMue: done15:58
TheMueniemeyer: ah, fine15:59
niemeyerTheMue: The last comment is wrong.. the one about s/anyone/it/15:59
niemeyerTheMue: It should be "any other charm" rather than "anyone"15:59
TheMueniemeyer: ok, will take care15:59
niemeyerTheMue: Cheers15:59
TheMueniemeyer: regarding the bundle url, shall it be a charm.URL too? so far it has been just a string16:05
niemeyerTheMue: No, that's not a charm URL.. it's a real URL16:06
niemeyerTheMue: http, https, whtaever16:07
TheMueniemeyer: got it, will take net/url.URL16:08
niemeyerTheMue: That'd be fine, yeah16:08
rogniemeyer: changes made. you might want to take a brief look before i submit as i've made the gocheck changes too.16:12
niemeyerrog: Nice, thanks16:15
niemeyerrog: LGTM16:15
rogniemeyer: thanks16:15
* niemeyer => lunch!16:16
niemeyerrog: np16:16
rogniemeyer: enjoy16:16
rogniemeyer: (submitted BTW)16:16
rogfwereade_, 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
rogfwereade_: (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
rogfwereade_: i mean "might break in practice on a heavily loaded machine"16:36
rogfwereade_: because it has to do a sleep to ensure that the right piece of code takes the lock at the right time16:36
rogfwereade_: there's no guarantee that it works16:36
fwereade_rog, ah, I missed that, hmm16:36
rogfwereade_: of course 1/20th of a second *should* be perfectly sufficient, but...16:37
fwereade_rog, indeed16:37
rogfwereade_: 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 them16:37
rogfwereade_: 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
rogfwereade_: 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
rogfwereade_: i'd add a fake test operation to zk.Conn which took the read lock, then waited for notification, then relinquished it.16:39
rogfwereade_: so then at least we could test the Close behaviour16:40
rogfwereade_: and all the operations are tested "by analogy". not great. but can you think of a better way?16:40
fwereade_rog, afraid not16:41
fwereade_rog, I think I'd prefer theoretically-unsound tests of the actual behaviour16:41
rogfwereade_: 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
rogfwereade_: and it can't test all of the operations either16:42
rogfwereade_: (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 sec16:44
rogfwereade_: afk?16:44
jimbakerrog, away from keyboard16:46
rogjimbaker: ah, thanks. hadn't seen that one before.16:46
rogjimbaker: good morning, BTW!16:46
jimbakerrog, thanks! and hopefully a nice evening to you! :)16:47
rogjimbaker: more like afternoon. evening approaches though.16:47
rogjimbaker: pretty grey day, to be honest.16:47
jimbakerrog, my impressions of newcastle, formed purely by literature, are always of a gray day, lashed by the winds of the north sea :)16:49
jimbakerthis may or may not have a basis in truth16:50
rogjimbaker: well, north sea about 20 east of here, but it can be gray. although it can be beautifully sunny too!16:50
rogs/20/20 miles/16:50
jimbakerrog, 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 imagery16:53
rogjimbaker: that is strong. we've had some fairly good winds here this season (our house faces the prevailing westerlies) but nothing like that16:54
jimbakerrog, 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 here16:56
jimbakerbut it's back to calm and sunny skies. the mountains made us aware of their presence, they are now back to quiet brooding16:57
TheMueniemeyer: https://codereview.appspot.com/5671055/ is in again17:00
niemeyerTheMue: cheers!17:30
niemeyerTheMue: TestCharm still needs dropping17:35
niemeyerTheMue: It's fully redundant with the test followin git17:35
niemeyerTheMue: Otherwise, LGTM17:35
TheMueniemeyer: oh, did i missed it? shit.17:37
TheMueniemeyer: 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
rogTheMue: why not leave that for the next one?17:38
TheMuerog: could do so too. only because it's a quick one.17:39
rogTheMue: your call17:39
TheMueniemeyer: what do you say?17:40
niemeyerTheMue: Works either way for me, as long as that's the end of it17:43
=== TheMue_ is now known as TheMue
TheMueGna! Connection lost.17:48
TheMueniemeyer: so, hopefully last try17:58
niemeyerTheMue: TestCharm is *still* there!?18:04
rogi'm off for the day, see y'all tomorrow18:05
niemeyerrog: Have a good one18:05
TheMueniemeyer: 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
TheMueniemeyer: my fault.18:06
niemeyerTheMue: All of the lines in TestCharm may be found in that precise shape in the test following it18:06
niemeyerTheMue: So there's no point in having it18:07
TheMueniemeyer: yep, i understand, only understood your review wrong18:07
TheMueniemeyer: i have been too much focused on the url18:08
niemeyerTheMue: Cool, np18:08
TheMueniemeyer: so one last propose ...18:09
niemeyerTheMue: Please just drop the test and submit18:09
TheMueniemeyer: ok, that's shorter ;)18:09
TheMueniemeyer: sadly you stole me some test methods today, so my high watermark is lower now :(18:10
TheMueniemeyer: 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
niemeyerTheMue: If you have changes in your branch, you necessarily must commit them, or revert if you don't want them in the final merge18:23
TheMueniemeyer: i committed my changes before the merge (it's the removing of AddCharm() )18:24
niemeyerTheMue: submit will deliver the changes that you did in your branch onto the target branch.. it will not do changes behind your back18:24
niemeyerTheMue: So why is the branch dirty?18:24
TheMueniemeyer: 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 there18:26
TheMueniemeyer: here i got no conflicts and a final go test in state worked fine18:28
niemeyerTheMue: Sure.. and then you have to commit that.. the merge will not be committed automatically18:29
TheMueniemeyer: ah, ok, that's missing. lbox help also states a commit, so i thought it's done automatically.18:30
niemeyerTheMue: It will commit the merge of your branch onto the target branch18:31
niemeyerTheMue: It will not fiddle with unfinished states in your own branch18:31
TheMueniemeyer: still not absolutely safe in the world of dvcs.18:34
TheMueniemeyer: thx for your help18:34
TheMueniemeyer: it's in18:39
niemeyerTheMue: Woohay!18:39
hazmatniemeyer, does lbox -cr support prerequisite branches?19:12
jimbakerhazmat, use the -req option19:15
hazmatjimbaker, thanks19:17
niemeyerhazmat: Kind of19:19
niemeyerhazmat: THis is my first TODO after the store work19:19
niemeyerhazmat: It won't diff the branches properly, so all but the first branch in the pipeline will contain data for the previous entries too19:19
niemeyerhazmat: So, in practice, it doesn't work very well yet19:20
niemeyerhazmat: 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 changing19:20
hazmatniemeyer, thanks for clarifying, i was noticing that problem but wasn't sure if -req was obmitted or would help.19:20
niemeyerhazmat: Now submit works, so I'll make submitting check to see if the pre-req is in19:20
niemeyerhazmat: and will make propose diff against the previous branch in the pipeline19:21
niemeyerhazmat: Will work on this as soon as store is out of the way19:21
hazmatniemeyer, sounds good19:21
niemeyerandrewsmedina: How're things going there?19:22
andrewsmedinaniemeyer: hi19:24
niemeyerandrewsmedina: Heya19:24
andrewsmedinaniemeyer: I started the local provider19:25
niemeyerandrewsmedina: Sweet!19:25
andrewsmedinaniemeyer: inspired by ec2 provider19:25
niemeyerandrewsmedina: Super19:25
andrewsmedinaniemeyer: but I had some problems with gocheck19:25
niemeyerandrewsmedina: Hmmm.. when did you do that?19:25
hazmatbcsaller1, is the subordinate spec ready for niemeyer to have another look?19:26
niemeyerandrewsmedina: Do you want an introduction to the development process via a low-hanging fruit?19:26
bcsaller1hazmat: yeah, thought it was in the queue, I'll check again19:27
andrewsmedinaniemeyer: low-hanging fruit?19:27
hazmatbcsaller1, it was proposed again to reitveld afaics19:27
niemeyerbcsaller1: Maybe it was and I just overlooked it.. please let me know of the URL and I'll check it out19:27
hazmater. wasn't19:27
niemeyerandrewsmedina: A simpler change that allows to see how things work19:28
niemeyerandrewsmedina: It's probably better than doing a lot of work without touching base as your first task19:28
andrewsmedinaniemeyer: It would be amazing :D19:28
hazmatbcsaller1, it looks like it was just pushed again to lp, lbox propose again should do the trick19:28
niemeyerandrewsmedina: This is a good candidate: https://codereview.appspot.com/5532098/diff/4001/juju/charm/metadata.py19:28
bcsaller1niemeyer, hazmat: it doesn't appear to upload the new push, I'll try it now19:29
bcsaller1must have gotten an error before I didn't see19:29
niemeyerbcsaller1, hazmat: Was that original spec merged already?19:29
niemeyerbcsaller1: Ouch19:29
niemeyerbcsaller1: What happened?19:29
bcsaller1kapil had to +1 it as well and then there were the changes you'll be looking at now19:29
andrewsmedinaniemeyer: what needs to be done?19:30
niemeyerandrewsmedina: We need to port that change onto the Go side19:30
niemeyerandrewsmedina: Should be a pretty comfortable ride19:30
niemeyerandrewsmedina: This will affect charm/meta.go19:30
niemeyerandrewsmedina: and the respective tests, of course19:30
andrewsmedinaniemeyer: how you set your dev enviroment and run the tests?19:30
niemeyerandrewsmedina: I do: export GOPATH=$HOME19:31
andrewsmedinaniemeyer: I did checkout in gopath19:31
niemeyerandrewsmedina: and "go get launchpad.net/juju/go/charm", for example19:31
andrewsmedinaniemeyer: and I run, tests module by module19:31
niemeyerandrewsmedina: This will set up all dependencies19:31
andrewsmedinaniemeyer: yes19:31
niemeyerbcsaller1: :-(19:31
niemeyerbcsaller1: We spent a lot of energy to agree on that state19:32
andrewsmedinaniemeyer I'm on the right way19:32
bcsaller1niemeyer: we talked about this at the call on Thursday19:32
niemeyerbcsaller1: 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 yet19:33
bcsaller1niemeyer: I saw them as related changes from the needed second +119:34
niemeyerbcsaller1: 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 uncommitted19:34
bcsaller1I have the full revision history. This is getting merged into the docs branch anyway, we can build whatever history you want there19:35
niemeyerbcsaller1: Do you have the link at hand?19:35
bcsaller1niemeyer: https://codereview.appspot.com/5541054/19:36
niemeyerbcsaller1: All I see in that link is "Thanks a lot for the extensive spec, and for keeping up with it Ben. Just a few19:36
niemeyerlast minute trivials."19:36
niemeyerbcsaller1: and hten "Please take another look."19:36
niemeyerbcsaller1: Where are the comments from Kapil that you are addressing?19:36
bcsaller1niemeyer: prior G+ talks , there are the changes brought up in the last meeting reflected in subordinate-internals19:37
niemeyerThis is really bad..19:37
niemeyerThis 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
niemeyerbcsaller1, hazmat:19:44
niemeyer  49          global/19:44
niemeyer  50               global/19:44
bcsaller1niemeyer: thats so that the path structure is the same for all types19:45
bcsaller1container/unit-0001/... or global/global/...19:45
bcsaller1the same path construction logic can then be used19:45
niemeyerbcsaller1: Sorry, I don't get..19:46
hazmatthat feels a little strange19:46
hazmatalso for containers the settings node should be under the container19:46
bcsaller1one is relation_scope_type/relation_scope/value19:46
bcsaller1err... one is relation_scope_type/relation_scope_value19:46
hazmatbcsaller1, but its redundant19:47
hazmatbcsaller1, global should suffice for scope and value19:47
bcsaller1hazmat: not really, it looks that way to a human, but its a uniform structure to the apis19:48
hazmatbcsaller1, the api can interpret global scope however it chooses19:48
hazmatbcsaller1, 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 path19:49
bcsaller1hazmat: agreed, I guess I didn't juggle it properly19:50
niemeyerbcsaller1: 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
niemeyers/don't we/do we/19:50
niemeyerbcsaller1: I assume we can tell whether a relation is a container relation or not based on the topology information, right?19:51
bcsaller1niemeyer: 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 structure19:51
bcsaller1niemeyer: yes, that has to be the case19:51
niemeyerbcsaller1: So why do we even have "container" or "global" nodes?19:52
niemeyerbcsaller1: What problem are these solving?19:52
niemeyerbcsaller1: In fact, what problem is the change from the original spec that was last reviewing solving?19:53
niemeyerlast reviewed, sorry.. I can't write anymore19:53
bcsaller1niemeyer: 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/019:54
niemeyerbcsaller1: Ok, I guess it enables watching more specifically..19:54
bcsaller1that as well19:54
niemeyerbcsaller1: as well? What else?19:54
bcsaller1it was very hard to understand container relations between services where neither of them was the container before19:55
niemeyerbcsaller1: How is that related to the structure of the nodes?19:57
hazmatit 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 structure19:58
bcsaller1hazmat: thanks, yes19:59
hazmatugh.. too much 'effectively'.. is that like a double negative ;-)19:59
niemeyerhazmat: I see, cheers19:59
hazmatbcsaller1, even given that.. the global/global can just be 'global', and settings should be moved under the scope20:00
niemeyerbcsaller1: So just s,global/global/,global/, please20:00
bcsaller1I 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 there20:01
niemeyerbcsaller1, hazmat: Hmm..20:01
niemeyerbcsaller1, hazmat: Can an individual principal have more than one subordinate under the same relation?20:03
niemeyerbcsaller1, hazmat: And vice-versa?20:03
bcsaller1container/unit-id/ will exist once for each container in the relationship20:04
bcsaller1subordinates can still only have once principal20:04
hazmatniemeyer, no.. they'd be different relations, relations are pair-wise service 2 service20:04
niemeyerhazmat: How many units may a given relation have?20:05
hazmatniemeyer, arbitrary20:05
hazmater. infinite20:05
bcsaller1limited by ZK memory ;)20:06
niemeyerhazmat: how might that happen when we're talking about container relations?20:06
hazmatniemeyer, it wont .. for container relations, its basically pair wise on unit 2 unit basis20:06
niemeyerhazmat: So you're saying that a given relation rel-000000042, will only ever have two units?20:07
hazmatniemeyer, no.. it will have n units.. where n is the number of primary/principal service units20:07
hazmater.. 2n20:07
hazmatwith the additional count coming from subordinates20:08
hazmatbut a given container scope for that relation will have two units20:08
niemeyerhazmat: Aha, ok20:08
niemeyerhazmat: That structure still feels a bit overblown20:09
niemeyerhazmat: Have you had a chance to see the branch that has the code reuse etc?20:10
bcsaller1niemeyer: he hasn't20:10
hazmatniemeyer, i have not, but i can envision it fairly easily20:10
niemeyerhazmat: Ok.. why do you want to move settings again?20:11
hazmatniemeyer, 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
niemeyerhazmat: Sounds good20:13
niemeyerhazmat: This means we can take both "global" keys out20:14
niemeyerhazmat: and that we can take the "container" keyword out as well20:14
niemeyerhazmat: So that relation-0000N has either:20:14
niemeyer1) A list of unit names, when it's a container relation20:14
niemeyer2) server, client, settings20:14
niemeyerThe latter, (2), happens to be exactly what one would find inside the unit name of (1).20:15
niemeyerSo global/global/, and container/, can both go away20:15
niemeyerhazmat: WDYT?20:15
hazmatniemeyer, 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 state20:17
hazmatiotw, works for me20:17
hazmatbcsaller1, what do you think?20:17
niemeyerhazmat: The scope comes out of the topology, IMO20:18
niemeyerhazmat: No matter what, the implementation shouldn't be _listing_ relation-0000N to know what to do20:18
hazmatniemeyer, agreed, topology is probably the right place20:19
bcsaller1I'm fine with that as well20:19
niemeyerhazmat: and it should already be there anyway right now..20:19
niemeyerbcsaller1: Super20:19
hazmatcool, sounds like agreement20:19
hazmatlet's ship it ;-)20:19
bcsaller1I'll make some revisions and push it again :-/20:19
=== Leseb_ is now known as Leseb
niemeyerbcsaller1: Sent these notes onto the review20:24
bcsaller1niemeyer: thanks20:24
niemeyerbcsaller1: np20:30
niemeyerI'm stepping out for some exercising outside.. will be back later today20:31
=== bcsaller1 is now known as bcsaller
hazmati'm gonna do the same, bbiab20:33
* hazmat yawns21:29
=== Leseb_ is now known as Leseb

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!