worker/peergrouper: new package

worker/peergrouper: new package

This will be a worker that maintains the replica set
with respect to the state.

The function in this CL implements the core functionality - it's
a stateless function that looks at a representation of the
current state of affairs and decides what the replicaset
member list should look like.

It may well change when faced with reality (although I've been
trying to sanity check with experimental code),
but I think it should be reasonable to check in now
as a staging post.

There are current some extraneous changes in this
branch (everything outside worker/peergrouper)
which are made redundant by other MPs. Please
ignore for the purposes of this review; I'll remove
before submitting.

https://code.launchpad.net/~rogpeppe/juju-core/479-desired-peer-group/+merge/201245

(do not edit description out of merge proposal)

[rog, 2014-01-10 19:07:31]

Please take a look.

[jameinel, 2014-01-13 11:37:11]

I didn't finish the review before I'm on the next call, but some thoughts I
didn't want to lose.

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired...
worker/peergrouper/desired.go:32: //func getPeerGroupInfo(st *state.State, ms
[]*state.Machine) (*peerGroupInfo, error) {
this function is commented out. Generally either we should delete it, or have
it. I'm guessing this is just part of the process as you're getting there.

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired...
worker/peergrouper/desired.go:58: return j
Is there really nothing like min that we can just use?

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired...
worker/peergrouper/desired.go:66: func desiredPeerGroup(info *peerGroupInfo)
([]replicaset.Member, error) {
Your description makes it sound like this function mixes a query (what peer
group do you want?) with an action (set the voting status of all the machines).

It would be best to split informational queries from side effects if possible.

[rog, 2014-01-13 17:40:12]

Please take a look.

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired...
worker/peergrouper/desired.go:32: //func getPeerGroupInfo(st *state.State, ms
[]*state.Machine) (*peerGroupInfo, error) {
On 2014/01/13 11:37:12, jameinel wrote:
> this function is commented out. Generally either we should delete it, or have
> it. I'm guessing this is just part of the process as you're getting there.

It is. I suppose I should probably delete it for this CL and bring it back in
the next one.

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired...
worker/peergrouper/desired.go:58: return j
On 2014/01/13 11:37:12, jameinel wrote:
> Is there really nothing like min that we can just use?

nope. it's not considered worth adding to the standard library.
(well, it wouldn't be one - there would be 12 of them).

https://codereview.appspot.com/49920045/diff/20001/worker/peergrouper/desired...
worker/peergrouper/desired.go:66: func desiredPeerGroup(info *peerGroupInfo)
([]replicaset.Member, error) {
On 2014/01/13 11:37:12, jameinel wrote:
> Your description makes it sound like this function mixes a query (what peer
> group do you want?) with an action (set the voting status of all the
machines).
> 
> It would be best to split informational queries from side effects if possible.

I've changed it so that it returns a map of values rather than setting the
machine fields in place. We want the caller to set the votes of the machines
after calling this function, and it's much easier to work out that information
here, rather than have the caller try to work out the info from the replicaset
members, which may not include all the machines passed in.

[rog, 2014-01-13 17:50:35]

Please take a look.

[natefinch, 2014-01-13 18:47:15]

LGTM overall.  I'd like to see comments on the functions even if they're not
exported... makes it a little clearer what each one is doing.

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired...
worker/peergrouper/desired.go:38: func desiredPeerGroup(info *peerGroupInfo)
([]replicaset.Member, map[*machine]bool, error) {
This function is pretty long, would be a little easier to read if it were broken
up more, I think.

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired...
worker/peergrouper/desired.go:199: found = m
break here?

[rog, 2014-01-13 19:35:20]

Please take a look.

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired.go
File worker/peergrouper/desired.go (right):

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired...
worker/peergrouper/desired.go:38: func desiredPeerGroup(info *peerGroupInfo)
([]replicaset.Member, map[*machine]bool, error) {
On 2014/01/13 18:47:15, nate.finch wrote:
> This function is pretty long, would be a little easier to read if it were
broken
> up more, I think.

Yeah, I'd toyed with that idea but not mustered up the necessary energy.

Done; definitely a good idea, thanks.

https://codereview.appspot.com/49920045/diff/80001/worker/peergrouper/desired...
worker/peergrouper/desired.go:199: found = m
On 2014/01/13 18:47:15, nate.finch wrote:
> break here?

Done.

[natefinch, 2014-01-13 19:44:37]

LGTM  The refactor makes a big difference.

[rog, 2014-01-14 09:23:08]

Please take a look.