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.