#juju-dev 2012-02-23 <fo0bar> ubuntulog test <TheMue> rog: morning <rog> TheMue: hiya! <rog> TheMue: feeling better, i hope <TheMue> rog: yep, yesterday already almost through with it, today feeling fine. thx. <rog> TheMue: cool <rog> TheMue: glad to hear it! <TheMue> rog: already proposed the next charm changes also containing your _ wishes. ;) <rog> TheMue: thanks <rog> TheMue: saw that <TheMue> rog: my pleasure <TheMue> rog: btw, got another 'infect'. <rog> TheMue: infect? <TheMue> rog: during studies i've learned scheme, but now i' trying lisp. ;) <rog> ah. CL? <rog> SBCL is the one to use <rog> baroque but interesting with it <TheMue> rog: yep, choosen sbcl as a start. <rog> TheMue: the hyperspec is the invaluable reference - i used it all the time <TheMue> rog: and i want to play a bit with actors in lisp, found some docs and a lib. <rog> TheMue: for your amusement, here was a little library i did in CL some time back: http://common-lisp.net/project/cl-xmlspam/ <TheMue> rog: ah <click> <rog> TheMue: i ported russ's <rog> oops <rog> i ported russ's C channel communications code to CL too. <rog> TheMue: never entirely satisfactory - it really needs lightweight threads, like Go. <TheMue> rog: i'll see if i can do something like actors running based on thread pools <wrtp> TheMue: so no first-class channels - sending messages to processes like in erlang? <TheMue> wrtp: yep, more like that <TheMue> wrtp: and like scala <wrtp> TheMue: right. <wrtp> TheMue: (i haven't looked at scala much) <TheMue> wrtp: i only for a short article i wrote more than a year ago, not very deep <wrtp> TheMue: the book to buy for getting into CL is "Practical Common Lisp", if you haven't already got it. <TheMue> wrtp: i dislike all those langs based on the jvm, they tend to be too weak due to the (wished) interaction with the java libs. <wrtp> TheMue: it's great. and doesn't try to hide the dubious bits <wrtp> TheMue: yeah. same with .net languages <wrtp> TheMue: i played with F# for a bit <TheMue> wrtp: full ack <TheMue> wrtp: i got "Land Of Lisp" for a review. but if the infect gets deeper i'll buy your book. ;) <TheMue> wrtp: i only read about f#, but it looked interesting too <TheMue> wrtp: and i still like erlang (used it for two years). <wrtp> TheMue: it's a classic. and extremely useful for finding out how to get around the bizarre file path handling for example <TheMue> wrtp: so, have to leave for a few minutes, banking. biab <wrtp> TheMue: c u <wrtp> something odd is going on with this machine: http://paste.ubuntu.com/853732/ <TheMue> back again <wrtp> TheMue: are you seeing codereview.com being slow currently? <TheMue> wrtp: no, today they have been fine. but had the too last week. * wrtp really doesn't like it when he makes lots of changes in response to a review... and then finds out they were made in the wrong branch. <niemeyer> yo! <wrtp> niemeyer: yoyo! <wrtp> niemeyer: responses made to both reviews, BTW. <wrtp> i hope you don't mind, i went for "k" rather than "pk" :-) <niemeyer> wrtp: Thanks! <niemeyer> wrtp: That's fine too <wrtp> cool <niemeyer> wrtp: Was just trying to make them consistent and distinct among themselves <wrtp> niemeyer: yeah, thought so. <wrtp> niemeyer: i like permKey BTW <niemeyer> wrtp: Sweet <TheMue> niemeyer: Already took a first look into https://codereview.appspot.com/5671055/? <TheMue> niemeyer: hiya btw <niemeyer> TheMue: Heya! <niemeyer> TheMue: Not yet, but you're my first priority today.. I'll just finish a meeting with Dave Cheney, and will be in your reviews <TheMue> niemeyer: great, thx <TheMue> niemeyer: just for info, weekly update leeds to zookeeper compiling troubles <TheMue> oh, wife calls for lunchtime <TheMue> biab <niemeyer> TheMue: The fix is already up for review <niemeyer> TheMue: and you'll have to upgrade to Go tip as well <niemeyer> TheMue: There's a bug in cgo itself that I fixed yesterday <TheMue> niemeyer: ok, thix, will switch to tip <wrtp> TheMue: actualy the fix is in weekly <wrtp> TheMue: (the newest weekly) <TheMue> wrtp: so tip should contin it too, shouldn't it? <wrtp> TheMue: sure. but i think we generally use weekly if it works. <TheMue> wrtp: hmm, if i recall it right, somebody named rog told me we use tip. <TheMue> wrtp: ;) <wrtp> TheMue: only occasionally, when things are broken with weekly :-) <wrtp> TheMue: ...as was the case on that previous occasion <TheMue> wrtp: thankfully an update-golang release|weekly|tip is enough for a full reinstall including the external packages <wrtp> TheMue: yeah, the go tool makes things much much easier. bliss. <wrtp> TheMue: i usually run all.bash though just in case the compilers have changed <TheMue> wrtp: me too, but it's inside the script above, together with the go get's <wrtp> TheMue: ah, it's your script. i though you just meant "hg update weekly" <wrtp> TheMue: (which does work usually in fact) <wrtp> s/though/thought/ <TheMue> wrtp: no, the script does the hg update, the all.bash and some clean go get <TheMue> wrtp: i'm lazy, ol' admin. ;) <TheMue> wrtp: doing something more than two, three times needs automation <wrtp> TheMue: what's the go get for? <wrtp> TheMue: to update to the required package versions? <TheMue> wrtp: yep, I earlier had troubles that once fatched code is only recompiled, not fetched again after release changes <wrtp> TheMue: yeah, i can see that. i don't usually have that problem though - i'll do get -u when a package stops working :-) <TheMue> wrtp: is a go build of own sources now intelligent enough to do a go get on 3rd party packages? <wrtp> TheMue: ... which makes it faster to switch versions. <wrtp> TheMue: a "go get -u" you mean? <TheMue> wrtp: yep, my script does a go get -u too <wrtp> TheMue: i don't think it's possible for it to know if the appropriate version has changed. <wrtp> TheMue: (of a given package, with respect to the current Go version, i mean) <TheMue> wrtp: would be nice. "hey, i'm building with rel 1 and import 3rd party pkg a. let's check if it has the according rel, otherwise update it on the fly." <wrtp> TheMue: i think if you do 'go get -u .' it should work <wrtp> lunchtime <TheMue> wrtp: yeah, it does, but you have to do it manually. <TheMue> wrtp: inside of a go build it would be nice <TheMue> wrtp: enjoy <niemeyer> Okay <niemeyer> TheMue: So, let me see your branch * niemeyer browses the review list <TheMue> fwereade_: thx for the review, the other branch with your comment is in https://codereview.appspot.com/5690051/ <fwereade_> TheMue, thanks; but I'm not clear which order they're meant to be merged in <fwereade_> TheMue, I'm sure I could figure it out but I'm lazy -- it would be good to set prereqs even if we don't yet get nice diffs out of it <TheMue> fwereade_: while point 1 is related to the testing dir question what exact problem do you have with .. in path? <fwereade_> TheMue, just that I'm suspicious that things will move :p <TheMue> fwereade_: yeah, understand, would be nice if the order/dependency could be seen easily <niemeyer> TheMue: Review sent <TheMue> niemeyer: thx for review. you get a compiling error? which kind? <niemeyer> TheMue: I don't think I said that <TheMue> niemeyer: ok, than i misinterpreted a comment <TheMue> niemeyer: so the state.Charm should implement the charm.Charm interface? <niemeyer> TheMue: Yes, and I think it already does <TheMue> niemeyer: hehe, maybe by accident <TheMue> niemeyer: it's the first time in state we're implementing an external interface, so i wondered <niemeyer> TheMue: Yeah, that's fine.. charm is kind of special in that we have several implementations of the interface <niemeyer> We have another one in the store as well <niemeyer> So 4 implementations right now * TheMue likes interfaces <niemeyer> TheMue: I like _Go_ interfaces.. specifically.. :-) <niemeyer> I've had not-so-great experiences before with other kinds of interfaces <niemeyer> Zope interfaces comes to mind, for instance.. <TheMue> niemeyer: never played with zope <niemeyer> I'll head to lunch, biab <rogpeppe> network has been playing up all day <rogpeppe> fwereade_: you gotta review <fwereade_> rogpeppe, tyvm <rogpeppe> fwereade_: you might not when you see all the comments! <rogpeppe> fwereade_: looking good though, in general <fwereade_> rogpeppe, heh, lots of comments beat just one saying "utterly hopeless; rejected, and shame on you" or equivalent :p <rogpeppe> fwereade_: lol <rog> right, i'm gonna reboot. <rog> well, ubuntu boot is quick, but shutdown... <fwereade_> rog, just a thought... if pstate were called pinfo, would you be happier about keeping path in there? <rog> fwereade_: i don't see why you want path in there really. it just saves a parameter in two functions. <rog> fwereade_: i'd be happier with a watcher type to gather together more params - i think things would look cleaner then. <fwereade_> rog, I admit that I originally put it in there because I was getting annoyed about the number of params ;) <fwereade_> rog, I'll experiment a bit <rog> fwereade_: if you pass watcher by value, you won't even need to incur an allocation - it'll be exactly to equivalent to passing the values as arguments. <fwereade_> rog, the tyvm holds, nice review <fwereade_> rog, I'm not so sure about the loop/goroutine question though <rog> fwereade_: no? <rog> fwereade_: i don't see why you need to start a new goroutine each time through <fwereade_> rog, forces are that (1) there are far more cases in which I want to break out of the loop than to continue looping, and (2) it seems to me that if I just recurse I'll eventually use an impolite amount of stack <fwereade_> rog, (1) hence not-quite-right as a loop, (2) hence goroutine <fwereade_> rog, sane or crackful? :p <rog> fwereade_: one mo, i'll paste the kind of thing i'm thinking of <rog> fwereade_: http://paste.ubuntu.com/854172/ <fwereade_> rog, that's basically what I had before I decided the loop was ugly (without the break loop, which doesn't feel to me like it really improves clarity) <rog> fwereade_: yeah, it looks better without the loop label: http://paste.ubuntu.com/854177/ <rog> fwereade_: but i think the loop is good - you're wanting to loop, so why not use one? <fwereade_> rog, hm, with for p.alive, that's definitely nicer <rog> fwereade_: oops, needs a p.alive=false in the DELETED case <fwereade_> rog, details details :) <rog> :-) <fwereade_> rog, what about awaitAlive though? the looping is definitely exceptional in that case * rog has a look <fwereade_> rog, hm, still, for consistency's sake I might change that one anyway <rog> fwereade_: i think you can use exactly the same structure to good effect: http://paste.ubuntu.com/854183/ <rog> fwereade_: they're both less lines of code, easier to read (IMHO) and more efficient... what's not to like? :-) <fwereade_> rog, yeah, exactly, with the loop conditions they are much nicer <fwereade_> rog, thanks :) <rog> fwereade_: np. (writing this kind of code with channels is nice, ain't it?) <fwereade_> rog, yeah, I'm really enjoying it :) <rog> fwereade_: good. that's what we want to hear :-) <rog> fwereade_: the main issue that probably needs discussion is whether unoccupying a node should delete it. <fwereade_> rog, ah yeah, I meant to mention that <rog> fwereade_: i'm tending towards the point of view that it shouldn't <fwereade_> rog, the raciness you sadly note has nothing to do with node clearing <fwereade_> rog, I don;t think it's sensible to assume that a presence node we want to watch will automatically exist <rog> fwereade_: if you were guaranteed that node existed, then you could just do GetW - no need for the ExistW call <fwereade_> rog, who says the Pinger even got started? <rog> fwereade_: i think the node should be created before the Pinger is started. <rog> fwereade_: so when some structure is created, a node is created for the watch node. and then a pinger and a watcher can rendezvous on it. <fwereade_> rog, well, it is, first thing on pinger creation, before run (so we can get any errors out quickly and easily, if they're going to happen) <fwereade_> rog, but... doesn't this just shuffle the raciness around? <rog> fwereade_: yeah, but there's no error if a watcher is watching a bad path <fwereade_> rog, who's responsible for creating that node? <rog> fwereade_: i don't think so. <rog> fwereade_: the state code. <rog> fwereade_: so if you're an agent, you can do an ExistsW if you like to determine when some structure comes into place (same way we watch the init node), and then run a pinger or a watcher on a node within the structure <rog> fwereade_: it's a slightly different approach i know, but i think it might make sense <rog> fwereade_: one mo, i've got to go and help carmen turn over the contents of a compost bin. back in 5. <fwereade_> rog, np, thinking <niemeyer> fwereade_: Can't awaitDead and awaitAlive be a single loop? <niemeyer> fwereade_: I don't have anything concrete yet, but my gut feeling about all those helper functions is that the problem itself shouldn't be so complex <fwereade_> niemeyer, hm, maybe, rog's suggested changes might make me able to do that clearly <niemeyer> fwereade_: I may be on crack.. so don't bother too much.. I'll do a more careful review on the logic once you're happy with it <fwereade_> niemeyer, still worth a look <fwereade_> niemeyer, cheers <fwereade_> niemeyer, I'm not sure that'd work tbh -- the timeout in awaitDead wouldn't really play nicely with everything else <fwereade_> niemeyer, or maybe I just can't see it :) <niemeyer> fwereade_: How's that any different from the timeout on awaitAlive? <fwereade_> niemeyer, what timeout? <niemeyer> fwereade_: The same that exists on awaitDead.. why is there a timeout on one and not the other? <fwereade_> niemeyer, the timeout is ony useful for detecting the case when updates have stopped <niemeyer> fwereade_: Why? <niemeyer> fwereade_: This isn't rhetorical, btw.. you may be right.. I'm brainstorming. <fwereade_> niemeyer, there's no timeout on awaitAlive because we don't need to do anything weird to support getting back a create/change at somepoint in the future <fwereade_> niemeyer, there's a timeout on awaitDead because that's the only way I could see to tell when a watch *hasn't* fired <niemeyer> fwereade_: How do I wait for an agent to come live? <fwereade_> niemeyer, ...call AliveW and wait for the channel to fire if it wasn't already alive? <niemeyer> fwereade_: Ok, sorry, I misunderstood what you meant by timeout then <fwereade_> niemeyer, ah, sorry :) <rog> back <rog> niemeyer: i'm thinking that all the logic becomes simpler if we assume that the node exists before starting. <rog> niemeyer: so rather than deleting the node at the end, we'd just set it to empty <niemeyer> rog: How can we assume it exists? <fwereade_> rog, even if we did, who would delete the node in the end? <rog> niemeyer, fwereade_: the node doesn't exist in isolation, surely? it's part of a larger zk structure that represents something, no? <niemeyer> rog: Sorry, I'm out of context and don't know what you're trying to say <niemeyer> rog: The node is within zookeeper.. it's not part of any structure <rog> niemeyer: i'm talking about a directory structure within zk <fwereade_> rog, I can see how it could work for machine nodes, but making it work for unit relation nodes is giving me a headache <niemeyer> rog: The node is within zookeeper.. it's not part of anything else, as far as the presence package is concerned <fwereade_> rog, and besides, I'd far prefer to make foo responsible for communicating foo's status without anyone else's help <niemeyer> fwereade_: It shouldn't make a difference <rog> niemeyer: that's fine. i'm just thinking that the initial waiting for the node to come into being can be done outside the presence package. <fwereade_> niemeyer, it's not creation that makes my head hurt so much as it is deletion <niemeyer> fwereade_: The presence node is context-less.. the package needs to know a path where it's supposed to exist, and it can assume that the parent exists, but nothing else. <niemeyer> fwereade_: What's the issue with creation? <fwereade_> niemeyer, there isn't one <niemeyer> fwereade_: There isn't one what? <fwereade_> niemeyer, an issue with creation <niemeyer> <fwereade_> niemeyer, it's not creation that makes my head hurt so much as it is deletion <niemeyer> fwereade_: Ah, I inverted the sentence <rog> fwereade_: why's deletion an issue? deletion would mean the node becomes dead as now. <niemeyer> fwereade_: You mean deletion is the problem.. what's the issue with deletion? <rog> fwereade_: (for instance when the containing directory and its children is deleted) <fwereade_> rog, niemeyer: who is responsible for it, and how do they arrange it with the unit agent, which may be behind schedule and have a bunch of other hook queued up before it can actually process a departure <rog> fwereade_: responsible for what? <fwereade_> rog, deleting the node <fwereade_> rog, I presume we don't want to keep nodes around forever just because something once used it <rog> fwereade_: the caller of the presence package. the idea is that responsibility for the node's creation and deletion is up to the caller. <rog> fwereade_: the presence package would purely manage pings on an existing node. <rog> fwereade_: i may be smoking more crack than usual, of course. <niemeyer> fwereade_: You only have to worry about it in the sane cases.. e.g. if Kill() is called <niemeyer> fwereade_: If there's a hard crash and the agent dies, there will be a bunch of trash around those nodes that has to be collected <niemeyer> fwereade_: The original code is obviously unable to do with it, since it may have gone forever <niemeyer> rog: The presence package should delete the node in the case where the call site is requesting the pinger to die in a way that notifies watchers <fwereade_> niemeyer, ok, unstated assumption: without a good reason, the same thing should handle creation and deletion. sane? <rog> fwereade_: yes <TheMue> niemeyer: i've got no prob dropping readCharm() and let Service use State directly. but in this case to keep it consistent i would like to change all entities from getting the ZK conn from state now to getting the State itself. <rog> niemeyer: i'd just set the contents to empty to do that <niemeyer> rog: No reason, I believe.. removal is a pretty good way to flag it <rog> niemeyer: rather than deleting the node. <TheMue> niemeyer: so i would change it in Machine, Unit and the now comming new ones too <fwereade_> rog, ok, but you're saying that (1) *something* unstated should create the node in the first place, and that other *something* should delete it at the end, and we shouldn't have a Kill (or at least not one that deletes the node) <fwereade_> rog, what would that something be? <niemeyer> TheMue: Yes, that's a good idea, but please do it in a follow up tiny branch that does just that <niemeyer> TheMue: Rather than piling up the unrelated change <rog> niemeyer: the nice thing about assuming that the node already exists is that you have some sanity check on whether it might ever change in the future, which we don't for a non-existent path. <TheMue> niemeyer: ok, this will be only the first step with Charm <TheMue> niemeyer: and Service ;) <niemeyer> rog: I don't understand what's the advantage. I agree with fwereade_. <niemeyer> rog: You seem to be saying that the nice thing about having someone else creating the node is that someone else has to do exactly the same logic that would otherwise be done inside the presence node, which to me makes no sense. <fwereade_> rog, yeah, niemeyer put it better than I managed <rog> i think i'm saying that other things know how to wait for existence (and that can imply other things too), but the pinger is the place that does keepalives. <rog> but i don't mind too much. it was just an idea. <niemeyer> rog: Still don't see what you mean.. the pinger is the thing that knows how to deal with a presence node to ping it, and that includes creation. <niemeyer> fwereade_: Btw, let's call StartPing as StartPinger os StartPinging.. StartPing feels weird <rog> niemeyer: it *can* include creation, but i don't think it has to. <rog> niemeyer: NewPinger seems ok to me... <niemeyer> rog: No, we can have a better interface as well and move things out of it <niemeyer> rog: and then we have a pinger that doesn't know how to ping. <niemeyer> fwereade_: Ok.. let's please have creation within the pinger.. there's no real argument that I can see to not do it. <rog> fair enough <fwereade_> niemeyer, I'm happy with that :) <niemeyer> fwereade_: And StartPinger, please.. <fwereade_> niemeyer, possible convention counterargument: time.NewTimer, time.NewTicker <rog> fwereade_: given that this is the presence package, i was just wondering if the "p" prefix is useful in pstate? just type state might be fine. and it makes the capitalisation less awkward. <niemeyer> fwereade_: Let's please use Start.. New lacks the idea that this is going to be alive doing something rather than just creating an object <rog> niemeyer: NewTicker?? <niemeyer> rog: I didn't name this one <fwereade_> niemeyer, fair enough <rog> niemeyer: it's seems a fine precedent to me, but there y'go. lots of New* functions create active things. <niemeyer> rog: I'm comfortable enough to improve existing conventions <jimbaker> hazmat, g+ mtg? <rog> niemeyer: i really don't think that's a great improvement. it's easier to remember when things stick to existing conventions. <niemeyer> rog: Sorry, let's move on <fwereade_> rog, I'm about to poke at pstate with a view to possible removal anyway :) <rog> do we have to use StartX for anything that invokes a goroutine? <rog> sorry, i'll stop <niemeyer> rog: When the next case shows up, we'll talk. <hazmat> jimbaker, sounds good <jimbaker> hazmat, cool, just invite djennings@example.org <hazmat> hmm.. invites out <hazmat> niemeyer, fwereade_, rog meeting time? <niemeyer> hazmat: Already joining <fwereade_> hazmat, noone here, I'll try again <wrtp> total failure <wrtp> niemeyer: i'm back online (on my apple, cos my laptop seems to have forgotten it can connect to a wireless network) and the hangout is full <wrtp> niemeyer, fwereade_, TheMue: so i guess you can tell me what went on :-) <niemeyer> wrtp: Oops :( <wrtp> niemeyer: i've no idea what's up with the laptop. it keeps on asking me for passwords (it should have them stored in the keyring) and doesn't connect anyway <wrtp> niemeyer: it's been funny all day in fact. rebooting didn't help. <wrtp> anyone got a handy hint for diagnosing ubuntu wireless connectivity problems? <Aram> you use network manager? <rog> it seems to be working again, woo * robbiew had to bail <hazmat> niemeyer, fwiw.. no bugs i can see log of hooks/scheduler.py http://pastebin.ubuntu.com/854310/ <niemeyer> hazmat: http://paste.ubuntu.com/854315/ <niemeyer> 111 looks like a fix <niemeyer> 127 too <niemeyer> 157 too <hazmat> niemeyer, what's that a log of? <niemeyer> hazmat: juju/hooks/ <hazmat> niemeyer, that's quite a bit more than the scheduler we where speaking of <niemeyer> hazmat: If you touch just scheduler.py, you won't get anywhere close to cleaning things up in the way I was talking about <niemeyer> hazmat: scheduler.py is a very particular interface that only makes sense with everything else in there <niemeyer> 202 is a fix too <niemeyer> 286 also <niemeyer> 312, 315, and 451.. <niemeyer> Yeah.. all of that is a side effect of a new code base being polished.. change the code base significantly, and these may not show up again, but others will.. <rog> right guys, i'm off now <rog> see ya tomorrow! <niemeyer> rog: Have a good evening <rog> niemeyer: and you <niemeyer> Cheers <hazmat> niemeyer, its clear we where talking about two different things, risk from a change, vs rewriting an implementation <niemeyer> hazmat: risk from (...) rewriting an implementation <niemeyer> hazmat: This is a gut suggestion.. I wouldn't rewrite one of the trickiest parts of an application that is working two months before a release that is supposed to be rock solid <niemeyer> hazmat: Your take <TheMue> back again <hazmat> niemeyer, its not clear you have the context on the problem or the solution that's under consideration.. your just pointing out an entire package of code, saying its had so many fixes and its tricky, be careful and don't touch it, while your saying how a rewrite will simplify things. yes change is a risk for stability, and in some cases not fixing a bug is the right call, but this is a fairly straight small forward change... and the change as i exampled i <hazmat> s to a part of the code that has been fine since day one... effectively its fixing a bug in a completely different package by adding a feature to the correct place. <niemeyer> hazmat: We're talking across each other indeed.. <niemeyer> hazmat: In the call, I pointed out what I'd like to do in Go, and you said it might be doable in Python too. I said it was a significant change for this point in time. That's what I'm talking about still. <niemeyer> hazmat: I'm not suggesting you don't fix a bug. <niemeyer> hazmat: In fact, I don't think I ever said anything close to "not fixing a bug is the right call". <niemeyer> In this context, of course.. in some cases it is the right call, surely. <hazmat> niemeyer, yeah.. i'm not considering rewriting that package, just fixing the bug.. the only rewrite at this point that's worth considering is the relation ephermal node watches via a backport of the pinger <niemeyer> hazmat: Phew.. we agree, after all :) <TheMue> niemeyer_: when can i expect the zookeeper fix? or is it just again a tag problem? <niemeyer_> TheMue: Tip is fixing, and the branch just has to be merged <niemeyer_> TheMue: I can do that right after the current meeting <TheMue> niemeyer_: great, thx <niemeyer_> TheMue: Submitting <TheMue> niemeyer_: wonderful, appreciate it <niemeyer_> TheMue: Done <niemeyer_> I'm heading out for some exercising.. probably back later #juju-dev 2012-02-24 <TheMue> re <niemeyer> Good morning all! <TheMue> niemeyer: hiya <fwereade__> heya niemeyer, you're on early :) <niemeyer> fwereade__: Yeah, a bit :) * rog finally makes his network connection work again <niemeyer> rog: Welcome back <rog> niemeyer: hi! <rog> niemeyer: you're up earlier than usual... <TheMue> rog: hiya <rog> TheMue: morning! <niemeyer> rog: Yeah, a bit <rog> niemeyer: any chance you could have another look at those CLs? i'm hoping to get something pushed this week. <niemeyer> rog: Not right now <rog> niemeyer: k <fwereade__> niemeyer, btw, there was a conversation the other day I think you missed about gozk panicing in C when a Conn is closed (and something else is busy in C code) <niemeyer> fwereade__: Ugh, that's not good <fwereade__> niemeyer, would fixing this just be a matter of a RWMutex on handle accesses, or is it more subtle? <niemeyer> fwereade__: Well.. I'd need some more information about the crash <fwereade__> niemeyer, I've seen it in a couple of cases, but the common feature is that some goroutine is always somewhere like: launchpad.net/gozk/zookeeper._C2func_zoo_wexists(0x36edf80, 0x36ef380) <niemeyer> fwereade__: Ok, do you have a full traceback? <fwereade__> niemeyer, here's the one I just saw <rog> niemeyer: i had a brief look at this - it just happens when zk derefs the nil handle, because handle access isn't mutexed, as fwereade__ suggestes <fwereade__> niemeyer, http://paste.ubuntu.com/855178/ <rog> suggests <fwereade__> niemeyer, the set line shouldn't be the problem, because that conn is not closed <niemeyer> fwereade__: Is this looping while you're calling Close on the zk Conn? <fwereade__> niemeyer, yes <niemeyer> fwereade__: That's going back to the point we talked about <niemeyer> fwereade__: We shouldn't do that <niemeyer> fwereade__: We shouldn't ever close a connection behind the back of logic that we're managing ourselves <fwereade__> niemeyer, ok, maybe I'm missing something <niemeyer> fwereade__: That's true for tests, and also for real logic <niemeyer> fwereade__: We can even implement locking to prevent the nil pointer reference at some point, but that's not the most critical issue <niemeyer> fwereade__: Ok, but do you recall that conversation? <fwereade__> niemeyer, ok, in that case I guess the tests I was trying to write are not actually relevant, because I'm really trying to test deal-with-session-event handling that doesn't exist yet <niemeyer> Feb 22 12:09:11 <niemeyer> fwereade_: We should never, ever, ever, leave background logic unattended while we're messing around with the system state in disruptive ways <niemeyer> fwereade__: Hmm, can you expand on that? <fwereade__> niemeyer, yes, I do; the reason I saw this one just now is because all I *thought* I had active on that conn was a watch <fwereade__> niemeyer, I was trying to verify that the watch channel got closed when the underlying ZK connection got yanked out from underneath <niemeyer> fwereade__: Ok, that's an interesting case.. maybe we have something to fix then <niemeyer> fwereade__: Can you please paste waitFor? <fwereade__> niemeyer, http://paste.ubuntu.com/855189/ <niemeyer> fwereade__: Sorry, I thought that was where the loop was.. TestDisconnectAliveWatch then, I guess <niemeyer> Ah, this is the guy blowing up: launchpad.net/gozk/zookeeper._C2func_zoo_set2(0x0, 0x36ef360) <fwereade__> niemeyer, yep <niemeyer> fwereade__: Please paste the test and (*Pinger).run <fwereade__> niemeyer, before I paste this: I know the stuff in the goroutine comment is wrong :p <fwereade__> niemeyer, <fwereade__> http://paste.ubuntu.com/855193/ <niemeyer> fwereade__: Cool :) <fwereade__> niemeyer, http://paste.ubuntu.com/855195/ <fwereade__> niemeyer, wait, you're right, it is set and not exists, bugger, I'm at least partially on crack (in a way that I was not hitherto aware of) <niemeyer> fwereade__: You know it's wrong in which way? <fwereade__> niemeyer, "this should be equivalent" <fwereade__> niemeyer, ie, what we should actually be doing is *not* just closing the conn until we've dealt with everything <fwereade__> niemeyer, but if it is indeed the set that's blowing up, anmd it looks like it is, then I'm completely confused because that's using a different conn to the one that gets closed <niemeyer> fwereade__: Not really <niemeyer> fwereade__: Oh, wait.. yes <fwereade__> niemeyer, so I would appear to be crackful in an entirely fascinating and original way :/ <niemeyer> fwereade__: But! <niemeyer> fwereade__: Please paste Close <niemeyer> fwereade__: I bet you have a race <fwereade__> niemeyer, oh balls, I try to hit the target node again <fwereade__> niemeyer, (to give clients as long a time as possible before they see a timeout) <fwereade__> niemeyer, (naively trusting that if the connection is borked it'll just error and I can ignore it) <niemeyer> fwereade__: That's fine <niemeyer> fwereade__: But a race is not fine.. please paste Close <TheMue> Interesting, http://cocode.io/beta/ will provide collaborative coding. It's written in Go knows Go as a highlighted lang. Sadly yet only GitHub, no Bazaar (or others). <niemeyer> <niemeyer> fwereade_: That's fine <niemeyer> <niemeyer> fwereade_: But a race is not fine.. please paste Close <niemeyer> fwereade_, fwereade__: Are you having wifi issues too? <niemeyer> fwereade_, fwereade__, fwereade: Are you having wifi issues too? :-) <fwereade> niemeyer, seems so :p <fwereade> niemeyer, http://paste.ubuntu.com/855206/ <fwereade> niemeyer, if you didn't see above <niemeyer> fwereade_, fwereade__, fwereade: I always knew you were not a single person.. <niemeyer> fwereade: Yeah, you have a race indeed <niemeyer> fwereade: Can you please have a look at this post: http://blog.labix.org/2011/10/09/death-of-goroutines-under-control <fwereade> niemeyer, I'm a globally distributed collection of outsourced drones, but don't tell anyone <niemeyer> fwereade: I'll highlight a few things afterwards, if they're not obvious <fwereade> niemeyer, cool, thanks * fwereade reads <rog> niemeyer: where's the race? <fwereade> niemeyer, I remember reading that when you wrote it, and totally forgot about it until now <fwereade> niemeyer, it seems clear, but I'd appreciate the highlighting all the same <rog> oh yeah, in TestDisconnectAliveWatch. i was looking in the pinger methods <fwereade_> grar <niemeyer> fwereade_: Are you here? :) <fwereade_> niemeyer, er, probably <niemeyer> fwereade_: Ok :) <niemeyer> fwereade_: So, quicklY! :-) <niemeyer> fwereade_: Observe the way Stop is implemented in the blog post <niemeyer> fwereade_: It sends a fatal error, and *waits* <TheMue> niemeyer: next round of https://codereview.appspot.com/5671055/ <niemeyer> TheMue: Thanks, I'll have a look at that next to rog's <TheMue> niemeyer: yep, thx <niemeyer> fwereade_: You're sending a stop signal, and ignoring the fact that, naturally, stopping isn't instantaneous <fwereade_> niemeyer, yeah; I had a "closed" channel I was waiting on originally, but it was pointed out that since closing was unbuffered I wasn't getting any benefit from waiting on closed after sending to closing <niemeyer> fwereade_: Uh.. this is bogus <niemeyer> fwereade_: Oh, or maybe not.. let me see again <rog> niemeyer: it looked ok to me :-) <fwereade_> niemeyer, I'm not saying there weren't subtle bugs in what I was doing, or in what I am doing ;) <rog> niemeyer: i *hope* it's not bogus otherwise my understanding of channels is fundamentally flawed... <niemeyer> rog: No, you're right.. as long it's unbuffered, that's fine <rog> phew <niemeyer> fwereade_: The race is in the test itself <rog> yup <niemeyer> fwereade_: No, sorry.. crack again.. <niemeyer> altConn is in a different end <niemeyer> Hmm <rog> niemeyer: isn't is a race that waitFor(c, altConn, path) is called concurrently with altConn.Close() ? <rog> (not that i've looked at waitFor) <niemeyer> rog: No.. the pinger is blowing up with a nil handle <niemeyer> Despite anything else, there's a race here in that the handle is being cleaned up underneath its feet <rog> niemeyer: that still looks like a race to me <niemeyer> rog: :-) <rog> niemeyer: even if it's not the thing that's blowing up <niemeyer> rog: If the logic in the test is sane, the Close won't happen before the kill <niemeyer> rog: if.. <rog> niemeyer: oh yes, i hadn't seen connect - i was assuming that the session event was the connection event, not the teardown event. <rog> but presumably connect waits for the first session event <fwereade_> rog, yes <niemeyer> rog: That's what I'm assuming too <rog> fwereade_: how reproducible is this? <fwereade_> rog, not very :( <fwereade_> rog, it'll happen eventually <rog> fwereade_, niemeyer: AliveW is busy on altConn at the same time that altConn is closed, right? <rog> so that looks like a race to me <fwereade_> rog, yes; so that Close could happen at any time, and that's definitely a problem <rog> i.e. the server gets killed, the goroutine receives the session event and closes altConn, at the same time AliveW happens to do a Change <fwereade_> rog, there are similar tests that really are *only* watching when the conn gets closed, and they're fine; yes <niemeyer> rog: If that's the case, then the connection is being Closed before kill() <rog> niemeyer: i don't think so <rog> niemeyer: the kill happens, then the close happens, then AliveW does something <niemeyer> rog: AliveW is run before kill.. <rog> niemeyer: AliveW has a goroutine <fwereade_> niemeyer, I guess it's not impossible that there's another session event confusing me <niemeyer> rog: OH, does it? <niemeyer> Why? <niemeyer> To transform the event, I guess <rog> niemeyer: yup <niemeyer> But it should be listening only <niemeyer> On a channel.. that's not supposed to blow up <niemeyer> fwereade_: Can you please paste the whole file, or just push the branch? <rog> yeah <fwereade_> niemeyer, sure <fwereade_> niemeyer, ~fwereade/juju/go-presence-nodes <fwereade_> niemeyer, when we're watching for aliveness we can't just dumbly watch a channel <fwereade_> niemeyer, we need to keep rewatching when we get changes before timeouts <niemeyer> fwereade_: Ho ho ho <fwereade_> niemeyer, technically, above, when we're watching for *deadness* <fwereade_> niemeyer, have I misunderstood something fundamental? <niemeyer> fwereade_: I'm just curious about the rewatching.. that may explain it <fwereade_> niemeyer, (also we may need to rewatch when waiting for liveness too, it's a bit of an edge case, but...) <niemeyer> fwereade_: Yeah, there's a race there.. <niemeyer> fwereade_: You're firing goroutines that use zk in a way that closing the connection will ignore completely <fwereade_> niemeyer, huh, I thought that closing the conn would close all the watches, and I'd see that <rog> niemeyer: i don't think there's a way to do it without either having a way to Close an AliveW or by making zk.Close concurrent-safe <rog> niemeyer: personally, i think i'd opt for the latter <rog> niemeyer: (after all, all the other routines are ok to call concurrently) <rog> s/routines/zk methods/ <niemeyer> rog: Yes, there may be something to change in gozk, but there's something to be handled in presence itself that I'm trying to understand <rog> niemeyer: presence seems ok to me, but i'm probably missing something <niemeyer> fwereade_: Why do we need awaitDead and awaitAlive to loop? <fwereade_> niemeyer, awaitDead to refresh the data watch until I get a timeout without the data watch having fired <niemeyer> fwereade_: Ah, right.. it's the opposite.. it fires when it doesn't get a watch <niemeyer> Hmm <fwereade_> niemeyer, awaitAlive to handle the case where a known-unresponsive node is deleted -- it's still dead but not in the same way and IMO I shouldn't really alert the client to say "yeah, nothing changed" <niemeyer> Well, looks like we need our Watcher type back.. <rog> niemeyer: if zk Close was concurrent-safe, everything would be fine <niemeyer> So that we can stop this logic in a sane way <rog> niemeyer: just like closing a net.Conn <rog> niemeyer: then the logic just stops as a matter of course <niemeyer> rog: What do you mean by concurrency safe? <rog> niemeyer: i mean that you could Close a zk conn concurrently with executing some other operation on the conn <fwereade_> niemeyer, it feels like one of the advantages of gozk is that the watches can just get auto-closed, and it would be really nice if we could follow the same convention here <fwereade_> rog, nitpick, technically it's all the other methods that aren't concurrent-safe ;) <niemeyer> rog: That's not enough <rog> fwereade_: they're concurrent-safe with each other... <rog> niemeyer: no? <niemeyer> fwereade_: Exactly <niemeyer> rog: What fwereade_ said <rog> niemeyer: i don't understand. wouldn't a mutex around the handle access be acceptable? <fwereade_> niemeyer, it still seems to me that a RWMutex on handle around C calls would be enough; what am I missing? <rog> niemeyer: (every method would have to respect it, of course) <niemeyer> rog: This is something else, not what you originally said <rog> niemeyer: that's what is necessary to make it ok to call Close concurrently, AFAICS <rog> niemeyer: which is what i was suggesting <niemeyer> rog: You're still missing the point.. <fwereade_> niemeyer, so am I I think <niemeyer> rog: It's not just about concurrency * rog usually does <niemeyer> rog: Close(); Set(foo).. BOOM! <rog> niemeyer: no. <rog> niemeyer: not if Set checks to see if handle is nil and then returns an error <niemeyer> rog: Dude.. please make up your mind <fwereade_> niemeyer, wouldn't we just grab a read lock, check for nil, and error back cleanly? <rog> fwereade_: exactly <niemeyer> fwereade_: YES! <niemeyer> fwereade_: We'd just do that.. that's not what was being said so far! <fwereade_> niemeyer, ha, I thought it was <niemeyer> <niemeyer> rog: What do you mean by concurrency safe? <niemeyer> <rog> niemeyer: i mean that you could Close a zk conn concurrently with executing some other operation on the conn <rog> niemeyer: i just said "make Close concurrent-safe". that doesn't imply that Close is the only method that needs to change. <niemeyer> rog: This is not about concurrency! Serial operations blow up! <rog> niemeyer: well, that's a different (although related) issue. <fwereade_> niemeyer, Close might block for a while (or *possibly for ever if enough is going on...) but that seems preferable to unrecoverable panics in C if if a hamfisted amateur like myself is trying to use the library <niemeyer> rog: Yes, it's the issue we're seeing <rog> niemeyer: i think we're seeing an issue because of concurrency, no? <niemeyer> <niemeyer> rog: This is not about concurrency! Serial operations blow up! <rog> niemeyer: is that the case in this example? i thought we were doing things in two separate goroutines <niemeyer> <niemeyer> rog: This is not about concurrency! Serial operations blow up! <niemeyer> rog: You can inline.. Close(); Set(); BOM! <fwereade_> rog, niemeyer: sorry, lamb chops are hot on the table, back shortly :) <rog> niemeyer: that's not what's happening in this issue though, right? <niemeyer> rog: Yes, it is <rog> niemeyer: in the same goroutine? <niemeyer> rog: No.. the fact there are two goroutines is completely irrelevant <rog> niemeyer: to my mind, it's not. if we were in a single goroutine it would be easy to avoid doing an operation after Close. <rog> niemeyer: it's the concurrency issue that makes it harder <niemeyer> rog: Sorry, but a "concurrency issue" is something else than what you have in mind <rog> niemeyer: ok, fair enough <rog> niemeyer: so... <niemeyer> rog: You can put a lock across every single function in gozk, and it will still blow up <rog> niemeyer: only if they don't check that handle is not nil before calling into C, no? <niemeyer> rog: Yes, and that's not a concurrency issue.. this is about using a connection after it's been closed. <niemeyer> rog: and make that not panic <rog> niemeyer: agreed. but just doing that won't fix our problem either. <rog> niemeyer: because then there would be a race. <niemeyer> rog: Sure, because you can't trust on the test for the handle to be valid without a lock <rog> niemeyer: exactly <niemeyer> fwereade_: So.. <rog> niemeyer: i think a RWMutex would probably do the job just fine <rog> on zk.Conn, that is <niemeyer> fwereade_: You're right, we must definitely fix gozk itself so that we protect from that kind of issue <niemeyer> fwereade_: I'm just still wondering if we should change the interface for that <niemeyer> fwereade_: I guess it's fine to keep it running it in background, and simply error out, given that this is purely a read operation without any side effects <rog> niemeyer: that was my thought. <niemeyer> fwereade_: Btw, I'd prefer to not have the (s state) as a parameter to the await functions <niemeyer> fwereade_: There's actually a bug in that regard in the current logic <niemeyer> fwereade_: No, there isn't, sorry <niemeyer> fwereade_: You're passing the parameter as a copy, which is safe <niemeyer> fwereade_: Still, I don't see why the parameter is necessary <rog> niemeyer: it saves passing the path and timeout parameters as separate arguments <rog> niemeyer: but i'm with you <niemeyer> rog: Yeah, I think the organization there needs some tweaking for clarification <rog> niemeyer: i wondered if things might be clearer if there was an internal watcher type which held the params that are being passed around. <niemeyer> rog: I was wondering about that too, but this is makes it seem like there wouldn't be much benefit compared to what is there now: <niemeyer> s, zkWatch, err = newStateW(conn, s.path) <rog> niemeyer: s, err := w.getStateW() ? <niemeyer> rog: The only bit that is actually constant is the path <rog> yeah <niemeyer> rog: Right, w is the path.. <niemeyer> rog: Which renders fwereade_ design elegant <rog> it was the other methods i was really thinking about (waitAlive and waitDead) <niemeyer> rog: Right.. as methods, they would have to live on a type that is simply the path <niemeyer> rog: Because timeout and aliveness changes <rog> niemeyer: that's fine. they can still be fields in the waiter type. it will change when they change. <rog> niemeyer: (it's not used concurrently) <niemeyer> rog: There's no point in having them as fields.. it's only used in that one function <rog> maybe "node" might be a better name for the type <niemeyer> rog: Yeah, path :) <niemeyer> rog: type node string, where node is the path <niemeyer> rog: That won't improve much what fwereade_ has there now, though. The design feels sound on that level * rog is thinking about it <niemeyer> rog: Well.. I guess we could have the connection on it too <niemeyer> Which would make it more interesting <rog> niemeyer: yeah <niemeyer> type node string <rog> niemeyer: and the watch channel to send to <niemeyer> func (n node) readState() (alive, timeout, error) <niemeyer> func (n node) readStateW() (alive, timeout, watch, error) <rog> func (n node) get() error <rog> sorry <rog> func (n *node) get() error <niemeyer> func (n node) awaitAlive <niemeyer> func (n node) awaitDead <rog> func (n *node) getW() error <niemeyer> rog: What's get? <niemeyer> rog: get error? :) <rog> niemeyer: gets the current state of the node. stores it in n. <rog> niemeyer: getW also gets a wait channel and stores that in n too. <niemeyer> rog: That's a bad name then.. update() maybe <rog> sure <niemeyer> and updateW() <niemeyer> rog: Yeah, that should improve things indeed, +1 <rog> cool * fwereade_ reads back... <niemeyer> rog, fwereade_: So.. I'd just like to take an eagles view on the whole issue to close it down, if that's ok <niemeyer> We have one bug, and one design approach we're agreeing that breaks a prior rule * rog feels those sharp eyes on him from far far above. <niemeyer> :) <niemeyer> The bug is, as we discussed at length, the fact closing and using a connection is a blow up in gozk for sure. We must fix that. <niemeyer> The design decision is a more subtle one <niemeyer> The prior goal I was personally attempting to ensure is that we never have background logic under our control running in parallel with other stuff that will break down the concurrently executing logic <niemeyer> Under that design approach, we'd need a Stop() method on the watcher <niemeyer> So that we can stop it.. that would avoid the gozk bug too <niemeyer> But, we're agreeing to not do that, and instead fix gozk, and establish a new condition: <niemeyer> If the background logic is entirely innocuous, it's fine to allow it to die a cold death <rog> sounds good to me. as long as the logic *will* die, and not be left around as garbage <fwereade_> niemeyer, that sounds sensible to me personally, but then I would say that <niemeyer> AliveW is one example of that.. by itself, it's doing nothing. If it blows up in the background, there are zero side effects, _as long as the logic that is using the watch does nothing_!! <fwereade_> niemeyer, this feels consistent with every other ZK watch <niemeyer> It's _NOT_ ok to have someone waiting on the watch resulting from AliveW, and doing further actions like changing the FS or whatever else, despite the fact the connection has been closed <rog> and the logic using the watch will just see a close on the channel, which means an error, so hopefully this kind of logic can cascade <niemeyer> fwereade_: Agreed <fwereade_> niemeyer, excellent <rog> fwereade_: agreed <niemeyer> rog: No, that's exactly what we don't want <niemeyer> rog: We'd have to reevaluate the use of the watch, if it's also in background <rog> niemeyer: no? i'm imagining some other innocuous logic layered on top of AliveW <niemeyer> rog: For it to be alright, it'd have to be innocuous too <rog> niemeyer: yes, definitely <niemeyer> rog: Cool, we're in agreement then <rog> yup <niemeyer> So, those are the relevant decisions <niemeyer> Glad we're in agreement, phew :) <niemeyer> fwereade_: In addition to that, we were nitpicking over the interface around the state type <niemeyer> fwereade_: Which is not terribly important, but that might be interesting to sort out if you still have the energy <fwereade_> niemeyer, I saw that and I'm not strongly invested in what I currently have; the fields seemed to go well together, and to stop the function signatures breaking lines, and so I decided it was good :) <niemeyer> fwereade_: rog was suggesting a node type, which feels nice to clarify the state handling <niemeyer> fwereade_: Something like this is the status quo of the discussion: <niemeyer> type node struct { conn, path, timeout, alive } <niemeyer> func (node) update() error <niemeyer> func (node) updateW() (watch, error) <rog> *node <niemeyer> Sorry, yes <niemeyer> func (*node) waitAlive() <niemeyer> func (*node) waitDead() <niemeyer> (the last two with the necessary return types and parameters) <fwereade_> niemeyer, LGTM <fwereade_> thanks guys, productive discussion :-) <niemeyer> fwereade_: waitAlive and waitDead would be implemented on top of the former two <niemeyer> fwereade_: Likewise! <rog> i keep on wondering if waitAlive and waitDead might not be better named as deadWait and liveWait respectively <rog> i.e. name them after the state they start in rather than the state they're waiting for. <rog> dunno <niemeyer> rog: -1.. these are nouns <niemeyer> we need verbs <rog> yeah <rog> i want to call waitDead waitDeath <rog> but can't think of an appropriate name for the other one <fwereade_> rog, I considered similar but "waitBirth" wasn't right <fwereade_> rog, indeed <niemeyer> waitAlive and waitDead feels great to me <rog> yeah <niemeyer> And on that node, I'll do some reviews.. <rog> that's fine. just thought i'd mention it in case others has similar thoughts. <fwereade_> niemeyer, would you like me to take a look at gozk as well? <niemeyer> Actually, let me finish answering emails first <fwereade_> niemeyer, I don't want to overload you :) <rog> i could do gozk <niemeyer> fwereade_: Yes, please <niemeyer> or rog.. whatever works <rog> or fwereade_ sure <rog> i'm a bit ahead of myself anyway currently * fwereade_ bows out gracefully <rog> fwereade_:
{'channel': 'juju-dev', 'month': 2}