[01:34] bcsaller, lots of comments on the rel type branch.. [01:34] hazmat: thanks, cool, I'll check it out [01:34] nothing structural [01:34] not finished [01:38] hazmat: yeah, didn't post yet. I do like that it will IM me [02:01] bcsaller, oh? i didn't know that it IMs [02:02] hazmat: when you log in with your google account I think its an account setting, you have to allow it access to your contacts or something [02:02] and then it shows up in google chat [02:06] nice [02:06] review out [05:09] anyone around who can help me debug stuff in the cloudinit provider please? [07:44] fwereade: morning [07:44] TheMue, heyhey [07:44] fwereade: new agent approach is in, now even more simplified [08:05] TheMue, reviewed, basically LGTM but with handwavey architectural notes [08:07] fwereade: what does "handwavey" mean? [08:07] TheMue, that I can't really justify my claims, even if they feel right to me, so I've waving my hands around in an attempt to convince by distraction [08:08] fwereade: hehe [08:09] fwereade: i'm currently testing a bit with private interface methods. so that at initialization only the surrounding instance has to be passed. [08:11] TheMue, yeah, I don't think there's anything wrong with your choices: I think you'll end up changing them in the future, but IMO nothing there is unclear or unpleasant and it can mutate to fit in with the rest of the app when we know *how* it needs to mutate [08:13] fwereade: the period and timeout are first shots, thats why i put them into constants. makes the tuning easier. could even be configurable in future [08:13] TheMue, yeah: those timings are good for testing [08:13] fwereade: and the embeddable type surely can be public if wanted. do you know any place today where the mixin is used outside the state lib? [08:14] TheMue, I may be wrong, but I thought the agent types themselves would use it and I was unjustifiably expecting them to live outside state [08:15] TheMue, btw total quibble: I'm not sure you need "embed" in the name -- it's a perfectly good agent class on it own, even if it doesn't have any behaviour other than connect/disconnect [08:17] fwereade: i've choosen it because it is no agent itself, it only provides something to embed for the work with/in agents [08:17] fwereade: like the current type is called AgentMixin [08:17] TheMue, like I said, total quibble :) [08:18] TheMue, yeah, I'm not saying "you must change this", just "I would have done it differently"; maybe some of my arguments will strike you as good things but I don't think they're dealbreakers :) [08:18] fwereade: yeah, currently there's no established way in go how to name a type wich is intended to be embedded only [08:21] fwereade: otoh AgentEmbed sounds indeed strange. [08:21] TheMue, random thought: AgentCore? [08:25] fwereade: core isn't used yet in the go port, but base. so i would prefer agentBase. and btw, todays mixin is only used inside state. so it's ok to let it be private [08:51] heya rog [08:51] er, wrtp :) [08:52] fwereade: mornin' [08:52] * wrtp doesn't seem to have a hangover but can't quite believe it yet. [08:53] wrtp, it's probably waiting in the wings :p [08:53] brb [08:53] fwereade: that's what i'm worried about. am continuing to drink water... [09:00] TheMue: i still don't understand quite how you intend agentEmbed to be used. on the one hand, it has a Pinger, which is something used by the agent itself; but on the other hand it calls Alive, which is something to be used by something outside the agent. [09:00] TheMue: i'm not sure that it makes sense to put both of these things in the same type. [09:00] TheMue: good morning BTW! [09:02] wrtp: moin ;) [09:03] wrtp: it's just used like the old AgentStateMixin [09:03] TheMue: i'm not sure that we need to faithfully copy every detail of the python structuring. [09:03] wrtp: it's embedded into Service, Machine, Unit (and maybe some more) [09:04] TheMue: as it is, it just looks to me like a very thin wrapper around the presence package. [09:04] TheMue: (presence + one method that does a timeout) [09:04] wrtp: dunno if any architectural changes are planned. afaik the first goal is to catch up with the python implementation and later make changes [09:05] TheMue: i think we're copying behaviour, not internal architecture. [09:05] TheMue: after all, this is quite a different language, and different structuring is often appropriate. [09:06] wrtp: it is, yes. but while presences is a technique regarding zk the wrapping is a more semantical encapsulation. otherwise we deon't need a state, every state user could use zk itself. [09:06] TheMue: i look at this code and i think "what does this actually *do*?". [09:07] wrtp: currently it's only demonstrating a way to discuss about [09:07] TheMue: as far as i can see, this is a convenience for stuff *inside* state, not an important visible abstraction. [09:07] wrtp: later that agentEmbed will be embedded in at least Service, Machine and Unit. And with that step they all will get the agent functionality. [09:08] TheMue: yeah, ok, i think i see it now [09:09] wrtp: it's just a pretty simple encapsulation that doesn't hurd but bundles logical behavior instead of reimplement those four methods inside of each type which needs it [09:09] TheMue: how about calling the type "agent" rather than agentEmbed [09:10] TheMue: then the various unit types can be UnitAgent, ServiceAgent, etc [09:10] TheMue: and they'll embed it [09:11] wrtp: fwereade and i already discussed about the naming. agendEmbed is indeed ugly, but only agent is also wrong. it "is" no agent. so i would prefer agentBase (or fwereades' agentCore). [09:12] wrtp: the embedding types are just stete.Service, state.Machine, state.Unit [09:12] TheMue: what about agentPresence? [09:12] TheMue: after all, that's what the type implements [09:13] wrtp: no, in this case all state types would have to be called UnitZK, ServiceZK, ... [09:13] wrtp: no information about "how" please, only about "what" [09:14] TheMue: i don't understand. isn't the type about the presence or absence of an agent? [09:14] TheMue: regardless of the fact it uses the presence package. [09:14] wrtp: the usage of those methods is btw spreaded. has_agent (now AgentConnected) is e.g. calles by juju.control.status [09:15] wrtp: it's about if an agent is connected or not. this naming has been a proposal by fwereade [09:16] TheMue, wrtp: I'm starting to lean towards just state.Agent myself [09:16] TheMue: connected==present, no? [09:16] fwereade, TheMue: i'm wondering if rather than embedding this type, each type (Service, Machine etc) could have a field or method which returns the Agent. [09:16] wrtp: yep [09:17] fwereade: then you'd call, e.g. svc.Agent().Connected() [09:17] rather than svc.Connected() [09:17] wrtp, that reads very nicely, yeah [09:17] wrtp: what are your concerns regarding embedding (aka mixin in py)? [09:18] wrtp: you /only/ win the additional (). [09:18] TheMue: i think the fact that the methods are named AgentConnected, WaitAgentConnected etc hints to me that this is really an independent type struggling to get free :-) [09:19] TheMue: if we don't embed, then we can use the more natural Connected(), WaitConnected(), Connect() and Disconnect() [09:20] wrtp: i can remove those Agent parts ;) [09:20] wrtp: every embedded type is an own type, nothing else, but only "embedded" [09:20] TheMue: yeah, but when the methods are directly on Unit etc then it's not entirely obvious that they're about the agent associated with the type. [09:20] wrtp: that's why i put Agent into the name ;) [09:22] TheMue: i'm not sure it's a big win to have it embedded. it means that, for instance, the agent methods will be documented several times. [09:22] (i think - i'll just check) [09:22] wrtp: your arguments match to all embedded types. they allways could be put in as extra field [09:22] TheMue: not when the embedded type is there to satisfy an interface, which is the usual reason for using them [09:22] wrtp: yeah, documented exactly where they are matching, at a Service, Unit, Machine [09:23] wrtp: … which is one (of many) use cases for type embedding, but it's not restricted to only this one. [09:23] wrtp: type embedding is the good old composition, nothing more [09:28] TheMue: i think that using them to satisfy interfaces is the usual reason. otherwise why embed rather than just adding a field or a method? [09:28] s/them/embedding/ [09:28] TheMue: BTW AFAICS if you have an embedded type, the embedded methods aren't documented at all. i thought they'd fixed that, but it seems not. [09:29] wrtp: but opposit of embedding/using an accessor we maybe shoud discuss with gustavo how this topic should be handled in future. the provided methods of todays mixin are only used in tests and at about 5 places in code. ;) [09:30] TheMue: sounds like a reasonable reason for not embedding it to me - no particular need to privilege those methods. [09:30] wrtp: good to know (the doc info). if you embedd a type you don't need an accessor method. and if you alternatively make the field public anyone could write to it (what could be unintended). [09:32] TheMue: that's not unusual. just because something is public doesn't mean you're allowed to write to it. (there are lots of examples in the core Go tree) [09:32] wrtp: it seems to me that you more have a problem that it is embedded than if it is logically working the right way or maybe should be organized a different way? [09:32] wrtp: which doesn't make it better (those exambles) for long term maintainability [09:32] TheMue: yeah, i'm happy with the functionality. i think it would work well (and be better documented) as an independent type. [09:34] TheMue: i'm concerned that if we embed it: a) we won't see any documentation for the Agent* methods at all or b) we'll get the same documentation comments repeated several times. [09:34] wrtp: btw, it IS an independent type also. there's no problem (beside the weird naming) with using it as a field [09:35] TheMue: cool. in which case why not just call it Agent, rather than assuming it's going to be embedded? [09:35] wrtp: the first argument is indeed bad, yes. multiple times wouldn't bother me, because i don't have to navigate to a different type to get the doc. [09:35] wrtp: is it an agent? [09:36] wrtp: or does it represent an agent? [09:36] TheMue: the latter, i think. [09:36] TheMue: it's all that the state package knows of agents, AFAICS [09:37] wrtp: what btw is an agent exactly? [09:38] TheMue: an agent is the program that runs on the machines or units etc and implements the connection between the state and the actual juju behaviour. [09:38] TheMue: so, Agent wouldn't *be* the agent, but it would represent one. [09:40] wrtp: still not absolutely happy with it [09:41] wrtp: i would like to know more about the motivation why the original code has choosen a mixin (opposit to a regular type). [09:41] wrtp: if that had no real reason i'm fine with those changes you've mentioned [09:42] fwereade: what do you think? was there a particular reason why the original code chose to use a mixin rather than a regular type? [09:43] wrtp, TheMue: it's common behaviour across a few types that aren't otherwise related; don't think there was any deeper reason than that [09:43] fwereade: do you think Agent as a name for the type seems reasonable? [09:44] func (u *Unit) Agent() *Agent [09:45] wrtp, yeah, I think so, package naming should take care of type ambiguity [09:45] wrtp, but I feel that this remains somewhat academic until we have a client [09:46] fwereade: agreed. [09:47] fwereade: i think it'll simplify the documentation though. rather than "agentEmbed is a helper type to embed into those state entities which..." we can have "Agent represents a juju agent." [09:47] TheMue: does that sound reasonable to you? [09:47] wrtp: it's ok for me to name it Agent (even if i'm still not absolutely happy with it). but mixins/emebeds are a nice way for my to encapsulate related functionality to weave it transparently into multiple types [09:48] wrtp: naming and doc change is ok [09:49] TheMue: embedding could still work ok if we embedded the Agent as a public field. i'm not sure that'll be the best idea, but it would allow the same access pattern. [09:50] TheMue: cool. sorry for my usual recalcitrance :-) [09:50] wrtp: no, please no public field. than i would better prefer an accessor [09:51] TheMue: fair enough. unit.Agent().WaitConnected() it is then :-) [09:51] wrtp: will change the proposal but will also ask gustavo [09:52] TheMue: sounds good [09:52] * TheMue still likes type embedding even w/o the needance of an interface implementation [09:55] * wrtp has been trying to look for examples where it's used like that. [10:07] fwereade: changed the timout value, but that's bad for tests. so i have to change it to be configurable [10:08] wrtp: we are 'before' Go 1, the language is young. so it will still last a long time until all good pattern/idioms are found [10:08] TheMue: true. [10:08] TheMue: but there is lots of good code already... [10:08] wrtp: yep [10:11] wrtp: and also lots of bad code [10:11] TheMue: of course, but not much in the Go tree as far as i've seen. that's my "gold standard"... [10:12] wrtp: and both will grow, with some/few pearls and a lot of bollocks [10:12] TheMue: sure. [10:39] bigjools, are you there? [10:47] TheMue, fwereade: trivial review for you: https://codereview.appspot.com/5845045/ [10:49] wrtp: LGTM, but what does IsError() additionally do? [10:50] TheMue: additionally? [10:50] TheMue: you mean "what does it do as well as check the particular zk error code?" [10:50] TheMue: or just "what does it do?" [10:52] wrtp: ok, better, what does it more than err = ...? [10:52] TheMue: zk errors are no longer simple integer types. [10:52] TheMue: check out the new version of zk. [10:53] wrtp: but error types, aren't they? [10:53] wrtp: will look later, have to leave for about two hours [10:53] wrtp: cu [10:53] TheMue: yeah, the thing you want to compare is the error code, not the struct containing it [10:53] TheMue: have fun [10:54] wrtp: ah, ok, thx [10:54] TheMue: i did originally name the function "HasErrorCode" but niemeyer suggested IsError [10:54] TheMue: which is a better name i think. === TheMue_ is now known as TheMue [11:21] wrtp, LGTM [11:22] fwereade: i think i'm gonna submit as it's so trivial [11:23] wrtp, sounds good to me [11:24] wrtp, were you working on an extension to testing to get States? [11:24] wrtp, or did you decide to leave it? [11:25] fwereade: i decided that testing shouldn't rely on the state package, but i am working on an extension to testing to clean the zk tree. [11:25] wrtp, sweet [11:27] fwereade: i'm wondering about exporting a ZkRemoveTree function from testing too, so we don't have *three* implementations... [11:27] wrtp, +10- [11:50] wrtp, how does https://codereview.appspot.com/5832045 look to you? [11:52] fwereade: it seems a little odd that the ContextId can't be inferred from the Context... [11:53] wrtp, I see ContextId as only meaningful in a map[string]*Context [11:53] wrtp, it's not like the Context ever uses the id itself [11:54] fwereade: i was thinking that perhaps it would work well if the context id was created in (and stored inside) the context itself. [11:54] fwereade: makes it easy for methods on the context to print it out in log messages too, which might be useful [11:54] wrtp, I don;t think the context id will be helpful in a log; the unit and relation definitely are, but the context id tells us nothing [11:55] wrtp, the members *might* be but that should be left up to the user, we don't want to print an arbitrarily long list of members every time [11:56] fwereade: can't we potentially have several contexts for a given unit/relation pair? [11:56] wrtp, the only meaningful way in which they will differ is in the members list [11:56] fwereade: they've also got state too, right? [11:57] wrtp, and what temporary/cached settings they happen to have [11:57] fwereade: yeah [11:57] fwereade: *shrug* [11:57] wrtp, yeah, but any State object will do; it's about the state of the env, not something unique to the context [11:58] fwereade: not quite, because there's temporary state that gets flushed (or not), right? [11:58] wrtp, (heh, where "env" refers to the global ZK environment) [11:58] wrtp, yeah, that's the map[string]*ConfigNode stuff I mentioned [11:58] wrtp, which remains, technically, speculation [11:59] fwereade: just seems like it might be a nice place to store the context id. we're always gonna have a 1-1 relationship of context<->id, no? [12:00] wrtp, why duplicate the information? it'll be in a map of context ids to contexts anyway [12:00] fwereade: in the past when i've had map[id]something, i've usually ended up putting the id in the something eventually. [12:00] wrtp, then we can put it in as soon as there's a need :) [12:00] fwereade: if you do that, you'll remove the field from ExecInfo, right? [12:00] wrtp, ofc [12:01] (that's a good enough reason for me, BTW) [12:01] wrtp, cool :) [12:06] wrtp, TheMue: I want to add something to testing, and I need opinions [12:07] wrtp, TheMue: specifically I want to move the test charms repo into testing and add some methods that always returns paths to copies of those charms (which should remain inviolate) [12:07] fwereade: sounds like a good idea to me [12:08] fwereade: make testing pull its weight a little more :-) [12:08] wrtp, TheMue: have me made a semi-firm decision to keep dependencies out of testing? [12:08] fwereade: not necessarily [12:08] wrtp, TheMue: because what'll actually be *convenient* 9 times out of 10 is "gimme a Charm", not a path-to-a-charm [12:09] fwereade: does that mesh well with the current charm testing code? [12:10] wrtp, depends: in go/charm itself, sometimes but definitely not always [12:10] wrtp, in terms of tests for state, and also for hooks (which will be using state) [12:10] wrtp, ...pretty much always, I think [12:11] fwereade: i'm happy either way. the testing package is all about convenience, and dependencies don't matter too much (although it's nice to keep them down as usual) [12:11] wrtp, well, ok, both charms and paths-to-charms will be useful in both situations [12:11] wrtp, cool, thanks [12:26] fwereade: you've got a reivew [12:26] review [12:26] wrtp, cheers [12:52] fwereade, TheMue: another review for your entertainment and delight: https://codereview.appspot.com/5841047 [13:00] wrtp, what would you think of renaming TestingT to Fatalfer? [13:00] lol [13:00] i wondered about that. [13:00] wrtp, my brain got tangled up trying t figure out what was actually in play there [13:00] wrtp, and since a gocheck.C would work just as well... [13:01] wrtp, ...the T feels wrong [13:01] fwereade: agreed [13:01] fwereade: but i couldn't think of a better name [13:01] wrtp, Fatalfer is ugly but effective [13:01] fwereade: true, and noone is ever gonna name it :-) [13:01] wrtp, IMO [13:02] ok, i'll change it [13:02] wrtp, yeah [13:02] wrtp, cool [13:03] wrtp, reviewed [13:04] fwereade: "Can we really delete "/" without getting an error?" [13:04] hmm, seems so! [13:04] wrtp, it's not in the tests [13:04] fwereade: ah, good point [13:04] fwereade: will add it [13:05] wrtp, cool, thanks [13:38] niemeyer: hiya! [13:38] Morning jujuers! [13:45] niemeyer, heyhey [13:46] niemeyer: moin ;) [13:46] TheMue: any particular reason why some methods are on *TopologySuite and some on TopologySuite? [13:47] wrtp: have to look, normally no reason. [13:48] TheMue: ok, was thinking of editing to make them consistent. [13:48] wrtp, LGTM [13:50] TheMue: another thing: why is TopologySuite in internal_test.go? i haven't looked too hard, but it looks like most of what it does is using public methods. you could maybe get away with putting readTopology into export_test.go, perhaps? [13:51] wrtp: it's only needed in SetUpTest() due to field modification [13:52] TheMue: what is? [13:52] wrtp: it's there because in the first approach the methods have been private [13:52] TheMue: ah, so it could go back outside internal_test? that would be a good thing to do, i think. [13:53] wrtp: hehe, that's the problem of asking a second question while the first isn't answered yet [13:53] TheMue: ah you mean *TopologySuite is only needed in SetUpTest? [13:53] :-) [13:53] wrtp: yep [13:53] i think i'd just make 'em all pointer-receiver [13:54] TheMue: it's pretty conventional to do that. [13:57] wrtp: could do this, yep, would change nothing beside the fact that any test could create side-effects by modifying the suite data by accident [13:58] TheMue: i don't mind that [13:58] wrtp: btw, the agent entity in the test will be removed if we agree that this approach is the right one. it has been there only for testing. [13:58] TheMue: ok. [13:59] wrtp: as i don't mind about not always use reference types [13:59] wrtp: otherwise, why aren't all automatically reference types, e.g. like in java? [13:59] TheMue: i usually go all one or all the other. [14:00] wrtp: what do you win with that approach? [14:00] TheMue: i usually use reference types when the struct is bigger than a few words. [14:00] TheMue: dunno really, just seems conventional. [14:01] wrtp: regarding agent, i would still like to keep it private. it's no standalone data type and will only be used directly inside the state package [14:01] TheMue: that's only true if it's embedded. i'm thinking that it'll be returned as an object in itself. [14:02] [09:44] func (u *Unit) Agent() *Agent [14:02] wrtp: one can call myUnit.Agent().Connected() w/o problems [14:03] TheMue: if the Agent method is exported, i think its return type should be. [14:03] TheMue: otherwise you can't declare a variable to assign it into. [14:03] wrtp: you don't need an extra variable [14:04] TheMue: you might want one... [14:04] wrtp: why? [14:04] wrtp: what's the use case? [14:04] TheMue: so you can use it some way that's independent of the unit its associated with. [14:05] TheMue: having a publicly exported method with a private return type seems a bit... rude. [14:05] TheMue: and then you won't see the docs for the methods either. [14:05] wrtp: that's why i preferred the embedded approach first [14:06] TheMue: i don't see why we can just export the Agent type. seems to work ok for me. [14:06] wrtp: Which methods would be in the Agent? [14:07] wrtp: so anyone can do an agt := &state.Agent{st, "/some/crude/path", nil} ? [14:07] niemeyer: https://codereview.appspot.com/5782053/diff/15001/state/agent.go [14:07] niemeyer: it's the reincarnation of the old AgentStateMixin [14:07] TheMue: only if you export the fields. [14:08] niemeyer: my second approach has been to create it as type that can be embedded [14:08] wrtp: ok, agt := &state.Agent{} [14:08] TheMue: that's true of any exported type. [14:08] TheMue: including State, etc [14:09] wrtp: yep, that's why i'm careful with exporting [14:09] wrtp: Agent only makes sense together with its state entity [14:10] TheMue: i disagree. it makes perfect sense for a unit agent to get the Agent from its Unit and use that independently. [14:11] wrtp: and forget about the unit state? [14:11] TheMue: (if it feels like it) [14:11] TheMue: yeah, why not? [14:12] TheMue: it'll be doing other things with the unit state, but it's not nonsense to use the Agent type independently. [14:12] TheMue: I suggest having those methods on the unit itself.. [14:12] niemeyer: hehe [14:12] TheMue: StartAgentPinger, IsAgentAlive, WaitAgentAlive.. [14:12] niemeyer: that's what i had with embedding [14:12] TheMue: You don't need DIsconnect.. just return the pinger [14:12] TheMue: No need to embed either.. this is the unit.. [14:13] niemeyer: only the unit? today it's also in machine and service [14:13] TheMue: pinger should be returned, rather than kept within the agent/unit [14:13] TheMue: Ah, bad assumption on my part.. nevermind then [14:13] niemeyer: you might prefer TheMue's original implementation: https://codereview.appspot.com/5782053/diff/7001/state/agent.go [14:14] niemeyer: we had a longer discussion about different variants this morning [14:14] TheMue: Yeah, just export Agent then.. [14:14] niemeyer: i thought that if this functionality was going to be used in several different places, that it would make sense as an independent type rather than embedding it. [14:14] TheMue: Sounds good, sorry for jumping in.. I'm clearly missing details [14:14] wrtp: +1 [14:14] wrtp, TheMue: What's there looks quite good, FWIW [14:14] niemeyer: hence the current proposal. [14:15] niemeyer: the latest version? or the earlier one? [14:15] niemeyer, wrtp: ok, will go in in a few moments as new proposal [14:15] wrtp: What's in https://codereview.appspot.com/5782053/diff/15001/state/agent.go [14:15] niemeyer: cool [14:16] niemeyer: i'm thinking that with the agent type exported, it'll work well. [14:16] niemeyer: but i'm not sure that TheMue agreeds :-) [14:16] agrees [14:16] wrtp: no pro, 3 to 1 ;) [14:17] TheMue: i'm hoping you'll agree too when you see it being used :-) [14:19] wrtp: nah, there only has been small doubts. i simply like embedded types. [14:19] me too... in their place :-) [14:22] wrtp: so i now can also remove the test type and integrate agent into unit as a first run [14:22] TheMue: sounds good [14:38] * wrtp goes to lunch. [14:40] fwereade: ping [14:40] niemeyer, pong [14:40] fwereade: Do you have a moment for a call? [14:40] niemeyer, sure [14:41] niemeyer, g+? [14:41] fwereade: Sounds good [14:43] niemeyer, invited [14:43] fwereade: You too :-D [14:43] niemeyer, ha, sorry, I'll join yours; ok? [14:43] fwereade: Ok, I'm moving, ok? [14:43] :) [14:43] HAHA [14:43] fwereade: Choose a random number, now! :-) [14:43] 1 [15:22] back [16:03] are we having a meeting now? [16:03] niemeyer, hazmat: ^ [16:05] wrtp, an hour from now I think [16:05] fwereade: my calendar said now. [16:06] fwereade: but i'm happy to go with an hour from now too [16:06] wrtp, niemeyer said "lunchtime" ;) [16:06] doh [16:06] fwereade: ah, that's fairly conclusive :-) [16:07] meeting in 1hr [16:07] i pushed it to the same time not taking into account aforementioned timezone shifts [16:07] on the calendar [17:01] * niemeyer waves [17:02] hazmat, fwereade, wrtp, bcsaller, jimbaker: Party time? [17:02] woooooo! [17:02] niemeyer: yay!!! [17:02] indeed [17:04] sorry, i need to find a better place. didn't realize we had a meeting now [17:04] invites should be out [17:04] I used to hangout with people.. nowadays I hangout with extras.. [17:05] haha [18:02] bizarre, i wonder where my windows have been disappearing to [18:03] they've all gone, including my editor (this IRC window, until i quit it from the sidebar and restarted it), my music player, etc [18:03] but they're still running! [18:03] guess i'll try restarting unity [18:03] again [18:05] weird, i did that and they've all come back .... except for Chrome which has now disappeared again [18:05] sigh [18:05] time to reboot, i think [18:06] and i can't type into any of them except this one! [18:12] niemeyer: that error will never happen unless something is doing a concurrent delete alongside ZkRemoveTree [18:12] niemeyer: because we'll get an error earlier when calling zk.Children [18:16] wrtp: That error will never happen unless it happens.. you've documented it to have a given behavior, so either implement the behavior or remove the comment. [18:17] niemeyer: fair enough [18:18] wrtp, I strongly fvour removing the comment [18:18] wrtp, I'd rather have that test fail than other tests work with weird inconsistent data [18:19] fwereade: i think it's important to be able to do ZkRemoveTree("/x") and have it work when /x doesn't currently exist [18:20] fwereade: it means that a test can be correctly torn down even if the path was not created as expected. [18:20] wrtp, should we not specifically be ignoring ZNONODEs though? [18:20] fwereade: The end result is the same if the node doesn't exis [18:20] t [18:20] fwereade: RIght, that's what's documented [18:20] wrtp, if there's any other error we want to know about it [18:20] niemeyer, ^ [18:21] fwereade: Yeah [18:21] fwereade: yeah. i am, but i hadn't taken into account the possibility that something might be deleting nodes concurrently with ZkRemoveTree [18:21] fwereade: that's what i do, i think [18:22] wrtp, ohhh... sorry, yes, you never get to delete if you got a ZNONODE out of children [18:22] fwereade: yup [18:24] wrtp, I guess we're unlikely to call that any time other than test teardown so it's unlikely anything else would be working concurrently [18:24] fwereade: that was my thought, but i'm checking anyway now, for completeness' sake. [18:25] wrtp, yeah, seeing two "unlikely"s in the same sentence makes me nervous ;) [18:25] fwereade: i'm not going to add a test though 'cos it's hellish difficult to test. [18:25] wrtp, I think I'm ok with that :) [18:25] fwereade: phew [18:29] wrtp: It's already quite awesome that we have testing for the testing functions at all :) [18:29] * wrtp smiles and nods. [18:30] it was easy to do and i wanted to check the thing worked! [18:38] niemeyer: submitted. thanks for the review. [18:43] wrtp, just saw a transient failure in TestStartAndClean: "cannot start ZooKeeper server: "cannot listen on port 21812: listen tcp 127.0.0.1:21812: address already in use" [18:44] fwereade: hmm [18:45] fwereade: i think perhaps there is a race because the server might hang on to the port for a little longer than it should after Destroy. [18:45]