=== Ursinha-afk is now known as Ursinha [00:10] menn0: thanks for that bomb [00:10] not [00:11] thumper: sorry :) [00:11] menn0: funny man :) [00:11] * thumper -> lunch [00:12] thumper, waigani: I initially going to make my email an "isn't it interesting that Dropbox is using Go too" email but couldn't resist when I saw that they had an errors package too. [00:13] from my brief look, juju/errors seems far more complete [00:16] anyone comfortable answering fairly specific watcher implementation questions right now? [00:20] waigani: you're OCR [00:20] https://github.com/juju/testing/pull/19 [00:20] +1, or -1 [00:23] davecheney: why not: MgoServer *MgoInstance ? [00:27] why make a pointer when we don't need to ? [00:27] MgoServer = &MgoInstance{} [00:27] is the same as [00:27] var MgoServer MgoInstance [00:33] what is Aram Havarneanu's irc nick? [00:33] (I'm going down the committer list for state/watcher.go, sorted by total commits) [00:33] jcw4: aram doesn't work for canonical any more [00:34] davecheney: thanks... you're coming up :) you're 7th in the list [00:34] jesus [00:34] that's scraping the bottom of the barrel [00:34] reminds me of a friend [00:34] haha [00:34] who bills himself as Canberra's 7th best Blues DJ [00:34] git log --format=%an state/watcher.go | sort | uniq -c | sort -nr [00:35] davecheney: lol [00:35] It looks like rogpeppe1 is offline for the last hour [00:35] TheMue: online? [00:35] jcw4: rog is in the UK [00:35] themue is in germany [00:35] davecheney: can you feel the cold breath? [00:35] jcw4: maybne you should just ask your question :) [00:36] okay... [00:36] jcw4: it's winter in australia, i have my own cold breath [00:36] davecheney: in the state watchers we have a common idiom [00:36] w.out is some sort of changes channel [00:36] in the watcher loop we have a local var of the same channel type [00:37] we expose w.out via a (w* ..Watcher) Changes() api [00:38] mmm [00:38] when new changes are detected, we assign w.out to the local out var [00:38] jcw4: can you point me to an example in the source ? [00:39] in state/watcher.go, any loop() implementation [00:39] I happen to be looking at actionWatcher around line 1596 [00:40] one of the select cases in the loop detects a read from the out channel [00:40] http://dave.cheney.net/2014/03/19/channel-axioms [00:40] the trick is here [00:40] case out <- ids.Values(): [00:40] ids = &set.Strings{} [00:40] out = nil [00:40] the last line basically says, [00:40] davecheney: right, that's what I'm getting to [00:41] remove this case from the select loop [00:41] davecheney: until a new change comes in right? [00:41] uip [00:41] yup [00:41] k. [00:41] so that's all context [00:41] that is the only place that assigns out again [00:41] my problem is [00:41] Changes() reads directly from w.out [00:42] mmm [00:42] so, for some reason, if I initially load the changes collection, the first read doesn't hit the out <- changes.Values() case [00:42] davecheney: var MgoServer MgoInstance: MgoServer has type MgoInstance (not *MgoInstance) - it is a zero initialised value, not a pointer. Correct? And arn't pointers cheaper to pass around than values? [00:42] waigani: we're never passing it around [00:43] oh [00:43] and then the consumer of the watcher gets the same changes the second time it reads w.Changes() [00:43] jcw4: ok [00:43] thereafter it works as expected [00:43] (I believe) [00:43] you're probably right [00:43] i don't know what "if I initially load the changes collection" [00:43] means [00:44] waigani: nobyd is passing MgoServer around, it's a package level variable [00:44] a singleton (if we used that word) [00:44] before the for { select {...}} loop I pre-load changes that occurred before the watcher was started [00:44] into the local changes set.Strings collection [00:45] davecheney: i.e. the expectation is that when you start a new Actions collection, you'll get an initial event notification of all the pending actions, and thereafter you'll be notified when new actions are added [00:46] jcw4: that sounds like the expected watcher pattern [00:46] what's happening is the pending actions are getting sent twice [00:46] davecheney: you are in error (a little) [00:46] davecheney: sorry, missed some context [00:46] * thumper steps away [00:48] davecheney: I just wonder if the local out var that we're using for signalling by setting to nil when the event is read shouldn't be an actual member on the watcher, and *that* is the channel exposed by Changes() [00:48] davecheney: otherwise we just say; duplicate initial events are to be expected... deal. [00:49] jcw4: can you show some code ? [00:49] i'm just not feeling it [00:49] davecheney: :) [00:49] davecheney: I don't blame you [00:49] davecheney: gimme a couple minutes [00:50] davecheney: right. So the definition of passing something = having a receiver for it in a func sig? Why are there no benefits to making a package level var a pointer? [00:50] ok, i'll answer the second part first [00:51] there is no justification for making it a pointer [00:51] so why ? [00:51] why allow the possiblity for someone to set it to nil [00:51] when we don't need to [00:51] the first part [00:51] i don't follow [00:51] can you say your first question again pls [00:52] davecheney: when do we get value from using a pointer instead of the value? [00:54] if I want to give you a copy of something, i pass by value [00:54] if I want to give you a reference to my thing, I pass a pointer [00:54] yep [00:54] but this is not entirely related to the receiver of a method [00:55] type V struct {} [00:55] func (v *V) F() { fmt.Println("%p", v) } [00:55] func main() { var v V ; v.F() } [00:55] F is defined on a *V, not V [00:56] but Go will altomatically take the address of v if needed, [00:56] so if you have a V and that V is addressable, go will do this [00:56] (*v).F() [00:56] in short: you don't need to declare everything as a pointer type just to call pointer methods on it [00:56] (there are exceptions, but they do not apply in this case) [00:56] oooh [00:57] davecheney: fwiw, my test code seems to deadlock [00:58] I didn't realise, I thought v.F() would fail because it's the wrong type [00:58] waigani: so, back at the change [00:58] where we had [00:58] var MgoServer = &MgoInstance{} [00:59] yep [00:59] that is declare a new MgoInstance value, take it's address and assign that to MgoServer [00:59] var can write [00:59] var MgoServer MgoInstance [00:59] davecheney: alternatively though, if you have a *V, it will not dereference to pass to a receiver of type (v V) [00:59] davecheney: right? [01:00] thumper: false [01:00] oh? [01:00] that is the same as saying [01:00] v1 := *v [01:00] v1.SomeValueMethod() [01:00] hmm... [01:00] lemmie check that [01:01] I thought it didn't [01:01] http://play.golang.org/p/VrpMKUYmwe [01:02] davecheney: I'm going to think more on these things... thanks for your help; I know I didn't articulate my question clearly :) [01:03] jcw4: show code, receive clarity [01:12] davecheney: So, just to check I've got the point: There is no point making MgoServer a pointer because, being a package level var, we are by default always referencing the same value whenever we use the var? [01:13] yes, that is true [01:13] but I don't think explains the point [01:14] davecheney: the more I look at it the more I'm convinced the problem is elsewhere http://paste.ubuntu.com/7762857 [01:14] there is no point in making the MgoServer a pointer, because there is no point in making it a pointer [01:14] lol [01:15] jcw4: as i read it [01:15] line 3 is ht eonly place where you can take things out of w.out [01:15] and line 36 is the only place you can put them into w.out [01:16] davecheney: okay; I kept reading line 36 as the case when w.out was being read [01:16] jcw4: you can write line 17 as [01:17] var out chan<- []string = w.out [01:17] if you like [01:17] that may make it clearer [01:17] davecheney: btw what is OCR? I think image parsing when I read that [01:17] On Call Reviewer [01:17] i didn't make that one up [01:17] blame, fwre [01:17] davecheney: I see to explicitly state it as a write only channel? [01:17] oh shit, am I OCR? [01:17] waigani: yup [01:18] man... [01:18] jcw4: if that helps explain what is going on [01:19] the watcher code is subtle and confusing [01:19] davecheney: it does clarify my understanding of that loop, but I'm still baffled as to how two calls to Changes() can return the same initial set of id's [01:19] davecheney: only the first two calls [01:19] the most subtle bit is the way we only allow one change event to escape the watcher at a time [01:20] jcw4: does your merge function work ? [01:20] davecheney: yes [01:20] I'll show code [01:21] davecheney: http://paste.ubuntu.com/7762872 [01:23] why is everyone passing *set.Strings ? [01:23] unrelated [01:23] but sort of related [01:24] a set.Strings is a map [01:24] and maps are already refernce values [01:24] davecheney: I thought of that when I read your other comments [01:24] unrelated [01:24] save it for another day [01:24] davecheny :) [01:25] jcw4: are you sure line 43 is every true ? [01:25] davecheney: ever true? [01:26] if id, ok := id.(string); ok { [01:26] if id isn't a string [01:26] then your merge function won't behave as expected [01:26] oh [01:26] there is a check [01:26] good [01:26] id' write that as [01:26] davecheney: afaik it is *always* a string, but if not it should die [01:27] switch id := id.(type) { [01:27] case string: [01:27] // do string things [01:27] default: [01:27] davecheney: +1 [01:27] fmt.Errorf("id is not of type string, got %T", id) [01:27] davecheney: my tests that pass : http://paste.ubuntu.com/7762882 [01:28] barf [01:28] how about just testing the function in isolation [01:28] you don't even need to make it a method [01:29] it can just be a function [01:29] you mean testing the merge() ? [01:29] yup [01:29] davecheney: that test is the overall watcher test, which seems to correctly handle queued actions before the watcher is started [01:30] this whole issue came up because bodie_ 's tests were failing: https://github.com/binary132/juju/blob/752f0b9232f7ef05b45a0efde7ba40cc4578409e/state/apiserver/uniter/uniter_test.go#L803-L825 [01:31] davecheney: I feel like I'm sucking you into my mess, and I need to isolate the problem further first [01:31] jcw4: the solution to that problem is to [01:32] log and issue, one per open question [01:32] davecheney: like a launchpad bug issue? [01:32] jcw4: yup [01:32] if there isn't an isue [01:32] then it never happene [01:32] afaiac [01:33] davecheney: okay - your clarification on the channels helped; thank you! [01:34] jcw4: np [01:34] i don't think i helped [01:37] davecheney: it did help; but I think I am looking in the wrong place for the problem [01:44] waigani: jcw4 https://github.com/juju/juju/pull/262 [01:49] thumper: OH MY GOD [01:49] davecheney: whazzup? [01:49] state/watcher.go +751 [01:50] davecheney: that actually is related to my problem I think [01:51] davecheney: if I do var out chan []string = nil, then bodie_'s tests actually pass, but my state ones fail, and I'm pretty sure my state ones are right [01:51] davecheney: (in actionsWatcher, not relationUnitsWatcher) [01:52] davecheney: I assume your deity name in vain use was about using out:=w.out and then immediately assigning it to nil? [01:53] jcw4: yup [01:53] :) [01:53] if that is actually part of the code [01:53] rather than just being lazy and not figuring out the right declaration for out [01:53] i'm a monkey's uncle [01:54] davecheney: I assumed it was just being lazy; hence my attempt to copy it used the var out chan []string = nil instead :) [01:56] :emoji hmmm [02:17] waigani: jcw4 https://github.com/juju/juju/pull/263 [02:22] davecheney: this assignment: id := id.(type) [02:22] davecheney: can you explain what it does exactly? [02:22] why does it reassign back into id? [02:23] the id inside the scope of the switch _case_ will be the asserted type [02:24] so the inner id shadows the outer id [02:24] I find that a little confusing that you are effectivly changing "id" to mean the "type of id" [02:24] I'd rather have a different name for it [02:24] true, but consider the alternative [02:24] swich id1 := id.(type) { [02:24] case string: [02:24] // do something with id1 [02:24] // do something else with id1 [02:25] // copy some code from another place an use id by mistake [02:25] // oh fuk [02:25] hang on... [02:25] I think I get it now [02:25] id inside the switch case shadows the original declaration of id [02:25] ok [02:25] right, id is still id [02:25] but it has a defined type [02:25] not interface [02:25] yup [02:25] yup [02:25] ok [02:25] I'm good then [02:26] cool [02:26] i agree it is a bit gross to deliberably shadow id [02:26] shadowing is contentious [02:26] I agree it is better than the alternative [02:26] but in this case, when you _want_ id to be of type string, or whatever [02:26] by shadowing the outer declaration you avoid the footgun of having that other id declaration floating around [02:27] I found it mildly confusing to work out what was the actual switch on [02:27] as it doesn't appear clear [02:27] to start with [02:27] you can also say (a bit piously) [02:27] i've asserted that id _is_ a string, so it should act like a string [02:27] but that sort of statement isn't helpful [02:28] ok, time for some lunch I think [03:02] thumper: thanks for the review. Comments addressed: https://github.com/juju/juju/pull/214 [03:03] kk [03:03] looking [03:05] if that case was a separate function, you wouldn't blink twice about the id parameter being a specific type [03:07] thumper: thanks very much [03:07] np [03:23] [LOG] 0:25.779 DEBUG juju.testing starting mongo in /mnt/tmp/test-mgo283910080 [03:23] [LOG] 0:25.780 DEBUG juju.testing found mongod at: "/usr/bin/mongod" [03:23] [LOG] 0:25.797 DEBUG juju.testing tls.Dial(127.0.0.1:60070) failed with dial tcp 127.0.0.1:60070: connection refused [03:23] [LOG] 0:25.798 DEBUG juju.testing tls.Dial(127.0.0.1:60070) failed with dial tcp 127.0.0.1:60070: connection refused [03:23] [LOG] 0:25.865 DEBUG juju.testing started mongod pid 15989 in /mnt/tmp/test-mgo283910080 on port 34290 [03:23] [LOG] 0:25.871 DEBUG juju.testing starting mongo in /mnt/tmp/test-mgo188237343 [03:23] [LOG] 0:25.872 DEBUG juju.testing found mongod at: "/usr/bin/mongod" [03:23] [LOG] 0:25.959 DEBUG juju.testing started mongod pid 16032 in /mnt/tmp/test-mgo188237343 on port 32859 [03:23] [LOG] 0:25.965 DEBUG juju.testing starting mongo in /mnt/tmp/test-mgo157584114 [03:23] [LOG] 0:25.966 DEBUG juju.testing found mongod at: "/usr/bin/mongod" [03:23] [LOG] 0:26.053 DEBUG juju.testing started mongod pid 16076 in /mnt/tmp/test-mgo157584114 on port 58000 [03:23] [LOG] 0:26.058 DEBUG juju.testing s [03:23] the test is using the WRONG versino of mongodb [03:23] unless I am mistaken, the test should be using [03:26] our special juju-mongodb, right ? [03:31] um... [03:31] I think it looks for either [03:31] * thumper shrugs [04:27] the replica set tests are getting worse [04:27] replicaset_test.go:69: [04:27] c.Assert(err, gc.IsNil) [04:27] ... value *mgo.QueryError = &mgo.QueryError{Code:0, Message:"couldn't initiate : can't find self in the replset config my port: 60785", Assertion:false} ("couldn't [04:27] initiate : can't find self in the replset config my port: 60785") [04:27] [LOG] 0:12.884 DEBUG juju.testing killing mongod pid 27146 in /tmp/test-mgo432122435 on port 60785 with killed [04:27] [LOG] 0:12.891 DEBUG juju.testing killing mongod pid 26447 in /tmp/test-mgo041410420 on port 40386 with killed [05:33] davecheney: fwiw, my problem earlier is related to the fact that in the state/apiserver/uniter tests the StartSync() method is called on the watcher which forces the watcher to load new events from the database, rather than waiting for notifications from the transaction log [05:34] :\ [05:34] i'm not sure either of those is a correct solution in the formal sense of the word [05:34] sounds like choosing the right hammer [05:34] :) [05:34] davecheney: I remember fwereade saying handling duplicate events should be expected [05:35] but when you're trying to write tests that are deterministic it's good to know all the paths that cause events [05:35] :) === vladk|offline is now known as vladk === uru-afk is now known as urulama [07:34] morning [07:53] morning TheMue [07:53] thanks for updating the calendar dimitern, I was running to eat before the alternate meeting time, but now I can relax a bit. [07:53] jam, :) yeah, some mix-up happened [07:55] jam: heya,I’m fighting with the bootstrap/endpoints bug. have been able to simulate it exactly once (first run), all other tries, even when removing everything, succeeded ??? [07:56] TheMue: I feel like its a race condition, so I would put sleeps in with what seems appropriate times. (either delay starting the API server, or delay the mongo start, or... ?0 [07:57] TheMue: were you able to get a traceback or any more information from the first failure? [07:57] jam: yep, that’s my current approach. I once had troubles with the dbus too, error seems to come here from mongo. [07:58] jam: no, only the wonderful output: ERROR EOF :( [07:58] TheMue: so if you grep labix.org/mgo/v2 for "EOF" you can see a bunch of tests that assert it: [07:59] // With strong consistency, it fails again until reset. [07:59] err = session.Run("serverStatus", result) [07:59] c.Assert(err, Equals, io.EOF) [08:00] jam: thanks for the hint, will take a look here too [08:01] TheMue: so it appears that EOF happens when the connection to mongo is broken, and you must issue a session.Refresh() in order to recover from it. [08:01] now... *what* would be causing us to disconnect from mongo? Maybe the new ensureMongoServer code? [08:01] Maybe ensureMongoServer is running while the API server is already up? [08:01] causing us to reset our connections. [08:01] If that seems true at all, then I would say we should not be starting the API server until ensureMongoServer has been run. [08:02] TheMue: Nothing concrete here, but IIRC, ensureMongoServer restarts the mongo db into single-user mode so that it can set up proper admin credentials before restarting again. === vladk is now known as vladk|offline === vladk|offline is now known as vladk [08:26] dimitern: what do you think about https://github.com/juju/juju/pull/255 [08:27] vladk, looking === rogpeppe2 is now known as rogpeppe [08:31] vladk, that looks fine, but we need the networker in place [08:32] looking for a review of this, please. anyone able to take a look? https://github.com/juju/charm/pull/11 [08:33] dimitern, jam, TheMue, urulama: ^ === rogpeppe is now known as rogpeppe1 [08:35] rogpeppe, how about peer relations? [08:35] dimitern: you can't make peer relations explicitly [08:36] dimitern: so i *think* it's ok to just ignore them [08:36] rogpeppe, so there can be no case in which the peer relation definition won't work? [08:37] dimitern: yeah [08:49] rogpeppe: reviewed === urulama is now known as uru-brunch [08:49] TheMue: thanks! [08:50] rogpeppe: yw [08:54] dimitern: are you reviewing the branch, BTW? [08:55] rogpeppe, sorry, got distracted and now i need to be in a call [08:55] dimitern: ok [08:59] dimitern: maas call [08:59] ? [09:13] dimitern: sorry I have to go pick up my son, apparently my wife didn't realize I had a call, bb in about 30 min... [09:13] jam, sure, np [09:41] dimitern: back === uru-brunch is now known as urulama [09:53] does the firewaller run on the machine agent only or on the state server as well? [10:00] dimitern, TheMue, jam: next one in the sequence, review appreciated: https://github.com/juju/charm/pull/12 [10:05] rogpeppe: will take a look [10:05] TheMue: thanks! [10:17] rogpeppe: reviewed [10:20] TheMue: thanks! [10:30] why juju when i try to deploy charm from local, takes somekind of old code, no the new one, that I changed? [10:31] Egoist, what commands are you running exactly? [10:32] juju deploy --repository="path to repo" local:precise/"charm name" [10:33] full example: [10:33] Egoist, it will help if you do $ tree /path/to/local/repo [10:33] dimitern: Why tree? [10:34] Egoist, the revision to deploy is discovered from the local repo dir by walking it recursively [10:34] Egoist, if the subdirs in the repo are in an unexpected order, the wrong revision will be picked, but you can always specify an explicit revision [10:35] Egoist, i.e. juju deploy --repository /path/to/repo local:precise/mycharm-5 [10:35] what do you mean by unexpected order? [10:35] will pick mycharm with revision 5 from the repo [10:35] what with something like this: [10:36] juju deploy --repository="charms" local:precise/mongodb [10:36] Egoist, unexpected, as in you might have multiple copies of the charm in the repo, some with duplicated revisions but different code inside [10:37] but i have only one copy of the charm [10:37] does it matter? [10:37] Egoist, when you run that, it should tell you Added charm local:precise/mongodb-42 [10:37] Egoist, that's why I asked to see a paste of the repo structure [10:38] whait i past you it [10:38] wait* [10:38] Egoist, thanks [10:38] please use paste.ubuntu.com or something similar, not directly in the channel :) [10:40] charms/ [10:40] └── precise [10:40] └── mongodb [10:40] ├── charm-helpers-sync.yaml [10:40] ├── config.yaml [10:40] ├── copyright [10:40] ├── hooks [10:40] │ ├── charmhelpers [10:40] │ │ ├── core [10:40] │ │ │ ├── hookenv.py [10:40] │ │ │ ├── host.py [10:40] │ │ │ └── __init__.py [10:40] │ │ ├── fetch [10:40] │ │ │ ├── archiveurl.py [10:40] │ │ │ ├── bzrurl.py [10:40] │ │ │ └── __init__.py [10:40] │ │ └── __init__.py [10:40] │ ├── config-changed [10:40] │ ├── configsvr-relation-changed [10:40] │ ├── configsvr-relation-joined [10:40] │ ├── database-relation-joined [10:41] │ ├── hooks.py [10:41] │ ├── install [10:41] │ ├── mongos-cfg-relation-broken [10:41] nooooooooooooooo [10:41] │ ├── mongos-cfg-relation-changed [10:41] │ ├── mongos-cfg-relation-joined [10:41] │ ├── mongos-relation-broken [10:41]