worker/peergrouper: implement worker

worker/peergrouper: implement worker It still lacks integration tests to actually check that the functionality works against mongo for real, but I think this is a reasonable stepping stone towards that. https://code.launchpad.net/~rogpeppe/juju-core/482-peergrouper-worker/+merge/208204 (do not edit description out of merge proposal)

[rog, 2014-02-25 18:46:24]

Please take a look.

[dimitern, 2014-02-25 19:19:04]

Looks good so far, but I haven't managed to go through the bulk of it (last 2-3 files). Some preliminary comments and thought below. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... File worker/peergrouper/desired_test.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... worker/peergrouper/desired_test.go:12: //	coretesting "launchpad.net/juju-core/testing" d https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... worker/peergrouper/desired_test.go:19: //	coretesting.MgoTestPackage(t) d https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... worker/peergrouper/desired_test.go:40: // Note that this should never happen - the mongo s/the// https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... File worker/peergrouper/mock_test.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:1: package peergrouper copyright + license header https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:36: type errorPat struct { what's wrong with errorPattern with a pattern field inside? https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:75: func errorFor(what ...interface{}) error { This is totally counter-intuitive (see also comments below). Can it be defined in a way that calling it later does not hurt? :) Like errorFor("Type.Func", arg1, arg2, arg3) instead of errorFor("Type.Func ", arg1, " ", arg2, " ", arg3) ? https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:94: errorPats = errorPats[:0] why not []errorPattern{} instead of slicing? https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:122: // checkinvariants checks that all the expected invariants s/checkinvariants/checkInvariants/ https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:168: if err := errorFor("State.Machine ", id); err != nil { shouldn't this be "State.Machine " rather than "State.Machine" ? If the extra space is needed only when there's an argument, can't errorFor add that space internally, so you don't have to remember doing it yourself? https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:279: func (m *fakeMachine) SetHasVote(hasVote bool) error { doc comment? https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:280: if err := errorFor("Machine.SetHasVote ", m.doc.id, " ", hasVote); err != nil { See - here it's even uglier - how about having a string first argument for errorFor and then a varargs list without spaces between the args; errorFor can process them internally. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:320: func (session *fakeMongoSession) CurrentMembers() ([]replicaset.Member, error) { doc comment? https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:327: func (session *fakeMongoSession) CurrentStatus() (*replicaset.Status, error) { ditto https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:366: // deepCopy makes a deep copy of any type by marshalling This is useful enough to be in utils/ somewhere. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:387: // watched for changes. Only one only one what? So.. we have yet another watcher look-alike here. Should this be in voyeur instead? https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:394: func WatchValue(val *voyeur.Value) state.NotifyWatcher { doc comment? https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/shim.go File worker/peergrouper/shim.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/shim.go... worker/peergrouper/shim.go:1: package peergrouper copyright + license header https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/shim.go... worker/peergrouper/shim.go:8: "launchpad.net/juju-core/instance" blank line before this https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.go File worker/peergrouper/worker.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.... worker/peergrouper/worker.go:1: package peergrouper c+l h https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.... worker/peergrouper/worker.go:8: "github.com/davecgh/go-spew/spew" i'm not sure we want yet another dependency just for debug logging. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.... worker/peergrouper/worker.go:62: // worker holds all the mutable state that we are watching. s/worker/pgWorker/ ? or the other way around perhaps. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... File worker/peergrouper/worker_test.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:1: package peergrouper c+l h https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:17: //type workerJujuConnSuite struct { delete commented code

[rog, 2014-02-26 13:58:08]

Please take a look. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... File worker/peergrouper/desired_test.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... worker/peergrouper/desired_test.go:12: //	coretesting "launchpad.net/juju-core/testing" On 2014/02/25 19:19:05, dimitern wrote: > d Reinstated instead. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... worker/peergrouper/desired_test.go:19: //	coretesting.MgoTestPackage(t) On 2014/02/25 19:19:05, dimitern wrote: > d Reinstated instead. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... worker/peergrouper/desired_test.go:40: // Note that this should never happen - the mongo On 2014/02/25 19:19:05, dimitern wrote: > s/the// Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... File worker/peergrouper/mock_test.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:1: package peergrouper On 2014/02/25 19:19:05, dimitern wrote: > copyright + license header Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:36: type errorPat struct { On 2014/02/25 19:19:05, dimitern wrote: > what's wrong with errorPattern with a pattern field inside? Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:75: func errorFor(what ...interface{}) error { On 2014/02/25 19:19:05, dimitern wrote: > This is totally counter-intuitive (see also comments below). > Can it be defined in a way that calling it later does not hurt? :) > Like errorFor("Type.Func", arg1, arg2, arg3) instead of errorFor("Type.Func ", > arg1, " ", arg2, " ", arg3) ? Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:94: errorPats = errorPats[:0] On 2014/02/25 19:19:05, dimitern wrote: > why not []errorPattern{} instead of slicing? Same difference. Almost the same amount of code, less garbage. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:122: // checkinvariants checks that all the expected invariants On 2014/02/25 19:19:05, dimitern wrote: > s/checkinvariants/checkInvariants/ Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:168: if err := errorFor("State.Machine ", id); err != nil { On 2014/02/25 19:19:05, dimitern wrote: > shouldn't this be "State.Machine " rather than "State.Machine" ? If the extra > space is needed only when there's an argument, can't errorFor add that space > internally, so you don't have to remember doing it yourself? Done as suggested above. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:279: func (m *fakeMachine) SetHasVote(hasVote bool) error { On 2014/02/25 19:19:05, dimitern wrote: > doc comment? Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:280: if err := errorFor("Machine.SetHasVote ", m.doc.id, " ", hasVote); err != nil { On 2014/02/25 19:19:05, dimitern wrote: > See - here it's even uglier - how about having a string first argument for > errorFor and then a varargs list without spaces between the args; errorFor can > process them internally. Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:320: func (session *fakeMongoSession) CurrentMembers() ([]replicaset.Member, error) { On 2014/02/25 19:19:05, dimitern wrote: > doc comment? Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:327: func (session *fakeMongoSession) CurrentStatus() (*replicaset.Status, error) { On 2014/02/25 19:19:05, dimitern wrote: > ditto Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:366: // deepCopy makes a deep copy of any type by marshalling On 2014/02/25 19:19:05, dimitern wrote: > This is useful enough to be in utils/ somewhere. I don't think it should be in utils - it's terribly inefficient. It could go into one of the testing packages, I suppose (testbase?) https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:387: // watched for changes. Only one On 2014/02/25 19:19:05, dimitern wrote: > only one what? > > So.. we have yet another watcher look-alike here. Should this be in voyeur > instead? I'm not sure. My only reservation comes from the fact that I don't want to make voyeur depend on the state package. I'll leave it here for the time being, if that's ok, pending a move to somewhere else. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:394: func WatchValue(val *voyeur.Value) state.NotifyWatcher { On 2014/02/25 19:19:05, dimitern wrote: > doc comment? Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/shim.go File worker/peergrouper/shim.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/shim.go... worker/peergrouper/shim.go:1: package peergrouper On 2014/02/25 19:19:05, dimitern wrote: > copyright + license header Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/shim.go... worker/peergrouper/shim.go:8: "launchpad.net/juju-core/instance" On 2014/02/25 19:19:05, dimitern wrote: > blank line before this Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.go File worker/peergrouper/worker.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.... worker/peergrouper/worker.go:1: package peergrouper On 2014/02/25 19:19:05, dimitern wrote: > c+l h Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.... worker/peergrouper/worker.go:8: "github.com/davecgh/go-spew/spew" On 2014/02/25 19:19:05, dimitern wrote: > i'm not sure we want yet another dependency just for debug logging. we definitely don't - it's a debugging remnant. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.... worker/peergrouper/worker.go:62: // worker holds all the mutable state that we are watching. On 2014/02/25 19:19:05, dimitern wrote: > s/worker/pgWorker/ ? or the other way around perhaps. We can't call it worker because of the clash with the package of that name. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... File worker/peergrouper/worker_test.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:1: package peergrouper On 2014/02/25 19:19:05, dimitern wrote: > c+l h Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:17: //type workerJujuConnSuite struct { On 2014/02/25 19:19:05, dimitern wrote: > delete commented code I've reinstated the test instead.

[rog, 2014-02-26 14:12:02]

Please take a look.

[dimitern, 2014-02-26 15:50:00]

The rest of the review. I like this and it LGTM, except for a few mostly minor suggestions. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/mock_te... File worker/peergrouper/mock_test.go (right): https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:236: func (m *fakeMachine) Refresh() error { ideally, we want doc comments for all exported methods below. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:407: // never return a non-nil error. The Wait method "always returns nil", according to its doc comment? https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:439: func (n *notifier) Err() error { doc comment? esp. if it always returns nil :) https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:448: func (n *notifier) Stop() error { here as well https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.go File worker/peergrouper/worker.go (right): https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:102: return newWorker(&stateShim{ so you need the shim only because the mongo port is not in state? https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:121: // N.B. we don't defer this call because Can you rephrase this a bit please? Why the panic? https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:145: logger.Debugf("pgWorker.loop looping") eww.. how about logger.Debugf("waiting events") or something? the logger is already prefixed by package, so no need to add pgWorker.loop IMO. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:153: logger.Debugf("update function returned error: %v", err) should this be just logger.Errorf("update function failed: %v", err)? https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:202: return nil, fmt.Errorf("cannot get replica set status: %v", err) So is it replicaset or replica set? Let's be consistent in all log messages. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:328: logger.Debugf("serverInfoWatcher.loop") "started server info watcher" ? https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:332: logger.Debugf("got infow change") "got server info change" ? https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:362: if !inStrings(m.id, info.MachineIds) { Why not just use: machineIds := set.NewStrings(info.MachineIds...) Then in the for loop use machineIds.Contains(m.id) and drop inStrings ? https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:376: logger.Debugf("machine not found: %v", err) Isn't this at least a warning? https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker_... File worker/peergrouper/worker_test.go (right): https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:156: errPat string s/errPat/pattern/ ? https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:157: err error Do you really need err here when it's always the same in the table below? Also, perhaps move the table inside TestFatalErrors if it's used only there. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:241: panic("unreachable") no need for this?

[fwereade, 2014-02-26 16:51:02]

I'm not 100% sure about the mocking strategy -- it feels like a simpler and dumber model might be clearer right now, but maybe the more tests we end up with the more it'll pay off. I did also find it a little hard to follow the code, and I *would* really like to see some of the other tests that STM to be implied by the existence of the unused initState param... but I'd also really like to see it land so we can start kicking it hard in reality and ferreting out the really weird cases (which will I fear be hard to simulate with the current mocking strategy -- eg ISTM that it's hard to directly test all the NotExists cases). Would appreciate your thoughts while I circle back round and see if I can catch myself out on any of the bits I only *think* I understand... https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.go File worker/peergrouper/worker.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.... worker/peergrouper/worker.go:48: // If we fail to set the mongo replicset members, s/replicset/replicaset/ https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... File worker/peergrouper/worker_test.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:44: func initState(c *gc.C, st *fakeState, numMachines int) { numMachines is not used... and I'd really love to see a test or two in which it actually is :).

[rog, 2014-02-26 16:58:09]

Please take a look. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/mock_te... File worker/peergrouper/mock_test.go (right): https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:236: func (m *fakeMachine) Refresh() error { On 2014/02/26 15:50:00, dimitern wrote: > ideally, we want doc comments for all exported methods below. i guess so, but since it's only exported to tests, and the entire raison d'être of these types is to implement the interfaces required by the worker, it seems somewhat overkill. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:407: // never return a non-nil error. On 2014/02/26 15:50:00, dimitern wrote: > The Wait method "always returns nil", according to its doc comment? yeah. i should just say "Wait and Err always return nil", except that's not true now I come to think of it because calling Err while the watcher is still alive will return tomb.ErrStillAlive. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/mock_te... worker/peergrouper/mock_test.go:439: func (n *notifier) Err() error { On 2014/02/26 15:50:00, dimitern wrote: > doc comment? esp. if it always returns nil :) There's not much point writing a doc comment if the type it's on isn't exported (i.e. it's invisible to godoc) https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.go File worker/peergrouper/worker.go (right): https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:102: return newWorker(&stateShim{ On 2014/02/26 15:50:00, dimitern wrote: > so you need the shim only because the mongo port is not in state? That's part of it (well, the mongo port is in state, but we don't have a mongo port for each machine), but we also need the shim because we want to mock out the watcher machinery and to cause arbitrary entry points to fail with an error. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:121: // N.B. we don't defer this call because On 2014/02/26 15:50:00, dimitern wrote: > Can you rephrase this a bit please? Why the panic? If there's a panic in pgWorker.loop (because there's a bug in the code) the defer will cause w.wg.Wait to be called, which will not return because the tomb has not been killed. What we want to happen is for the panic to be printed and the worker to exit and be restarted. I've rephrased. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:145: logger.Debugf("pgWorker.loop looping") On 2014/02/26 15:50:00, dimitern wrote: > eww.. how about logger.Debugf("waiting events") or something? the logger is > already prefixed by package, so no need to add pgWorker.loop IMO. debugging remnant - deleted. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:153: logger.Debugf("update function returned error: %v", err) On 2014/02/26 15:50:00, dimitern wrote: > should this be just logger.Errorf("update function failed: %v", err)? more debugging remnants, also deleted. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:202: return nil, fmt.Errorf("cannot get replica set status: %v", err) On 2014/02/26 15:50:00, dimitern wrote: > So is it replicaset or replica set? Let's be consistent in all log messages. I think in english sentences when not referring to the package, it should be "replica set". I've changed some comments to be consistent with that. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:332: logger.Debugf("got infow change") On 2014/02/26 15:50:00, dimitern wrote: > "got server info change" ? more debugging remnants, deleted. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:362: if !inStrings(m.id, info.MachineIds) { On 2014/02/26 15:50:00, dimitern wrote: > Why not just use: > > machineIds := set.NewStrings(info.MachineIds...) > > Then in the for loop use machineIds.Contains(m.id) and drop inStrings ? I can't get excited about it. They're both about as readable as each other and set.NewStrings will create more garbage and be less efficient in the expected case. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:376: logger.Debugf("machine not found: %v", err) On 2014/02/26 15:50:00, dimitern wrote: > Isn't this at least a warning? Done. And I've added the missing return too - one less possible panic... https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker_... File worker/peergrouper/worker_test.go (right): https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:156: errPat string On 2014/02/26 15:50:00, dimitern wrote: > s/errPat/pattern/ ? Done. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:157: err error On 2014/02/26 15:50:00, dimitern wrote: > Do you really need err here when it's always the same in the table below? Done. > Also, perhaps move the table inside TestFatalErrors if it's used only there. I prefer not to clutter the test function with all the table data. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:241: panic("unreachable") On 2014/02/26 15:50:00, dimitern wrote: > no need for this? we do need it because the go compiler doesn't know that c.Fatalf never returns.

[rog, 2014-02-26 17:13:03]

Please take a look. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.go File worker/peergrouper/worker.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.... worker/peergrouper/worker.go:48: // If we fail to set the mongo replicset members, On 2014/02/26 16:51:02, fwereade wrote: > s/replicset/replicaset/ Done. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... File worker/peergrouper/worker_test.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker_... worker/peergrouper/worker_test.go:44: func initState(c *gc.C, st *fakeState, numMachines int) { On 2014/02/26 16:51:02, fwereade wrote: > numMachines is not used... and I'd really love to see a test or two in which it > actually is :). It's just a mistake that it isn't used - now fixed. Are you saying you'd like to see a test in which we start with a number of machines that isn't 3? Or are you reading something else into the numMachines parameter?

[natefinch, 2014-02-26 21:10:29]

I think some of my comments are out of date, like the commented out coretesting one, so feel free to ignore anything that has since been fixed up. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired.go File worker/peergrouper/desired.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... worker/peergrouper/desired.go:260: mid, ok := member.Tags["juju-machine-id"] not a big deal, but mid is perhaps a poorly chosen abbreviation. I was trying to figure out how the variable represented the midpoint of anything.... only realizing after a little bit that it just means machine-id. Why not just use id, since there's no other variables of that name? https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... File worker/peergrouper/desired_test.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... worker/peergrouper/desired_test.go:12: //	coretesting "launchpad.net/juju-core/testing" I presume this is a mistake https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.go File worker/peergrouper/worker.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/worker.... worker/peergrouper/worker.go:8: "github.com/davecgh/go-spew/spew" On 2014/02/25 19:19:05, dimitern wrote: > i'm not sure we want yet another dependency just for debug logging. I also wonder if we shouldn't be careful about depending on people's personal github accounts that might just go away. At least the labix stuff is under the control of a Canonical employee. https://codereview.appspot.com/68650044/diff/60001/utils/voyeur/value.go File utils/voyeur/value.go (right): https://codereview.appspot.com/68650044/diff/60001/utils/voyeur/value.go#newc... utils/voyeur/value.go:68: func (v *Value) Closed() bool { This is so much better, thanks. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.go File worker/peergrouper/worker.go (right): https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:47: type notifyFunc func() (bool, error) Can you name the returns? (changed bool, err error) I think that would make it a lot clearer without having to read the whole doc string. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:78: // purposes only. "It is an interface so we can swap out the implementation during testing." https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:81: // When something changes that might might affect one too many mights https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:165: if _, isReplicaSetError := err.(*replicaSetError); !isReplicaSetError { I'd prefer this check to be in its own function even if it's not called anywhere else. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:194: // getPeerGroupInfo collates current session information about the comment/function mismatch (get)PeerGroupInfo

[rog, 2014-02-27 09:17:14]

Comments addressed in a later branch. https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired.go File worker/peergrouper/desired.go (right): https://codereview.appspot.com/68650044/diff/20001/worker/peergrouper/desired... worker/peergrouper/desired.go:260: mid, ok := member.Tags["juju-machine-id"] On 2014/02/26 21:10:29, nate.finch wrote: > not a big deal, but mid is perhaps a poorly chosen abbreviation. I was trying > to figure out how the variable represented the midpoint of anything.... only > realizing after a little bit that it just means machine-id. Why not just use id, > since there's no other variables of that name? I avoided "id" because there are two kinds of ids around here, member ids and machine ids. But you're right, "mid" doesn't even help there. Renamed to machineId in a subsequent branch. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.go File worker/peergrouper/worker.go (right): https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:47: type notifyFunc func() (bool, error) On 2014/02/26 21:10:29, nate.finch wrote: > Can you name the returns? (changed bool, err error) I think that would make it > a lot clearer without having to read the whole doc string. Done. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:78: // purposes only. On 2014/02/26 21:10:29, nate.finch wrote: > "It is an interface so we can swap out the implementation during testing." Done. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:81: // When something changes that might might affect On 2014/02/26 21:10:29, nate.finch wrote: > one too many mights Done. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:165: if _, isReplicaSetError := err.(*replicaSetError); !isReplicaSetError { On 2014/02/26 21:10:29, nate.finch wrote: > I'd prefer this check to be in its own function even if it's not called anywhere > else. I think we should be allowed to write inline type assertions. A extra function doesn't add much here AFAICS. https://codereview.appspot.com/68650044/diff/60001/worker/peergrouper/worker.... worker/peergrouper/worker.go:194: // getPeerGroupInfo collates current session information about the On 2014/02/26 21:10:29, nate.finch wrote: > comment/function mismatch (get)PeerGroupInfo Done.