|
|
|
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)
Patch Set 1 #Patch Set 2 : worker/peergrouper: new package #
Total comments: 6
Patch Set 3 : worker/peergrouper: new package #Patch Set 4 : worker/peergrouper: new package #Patch Set 5 : worker/peergrouper: new package #
Total comments: 4
Patch Set 6 : worker/peergrouper: new package #Patch Set 7 : worker/peergrouper: new package #
Total messages: 8
|
rog
Please take a look.
|
12 years ago (2014年01月10日 19:07:31 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||
Please take a look.
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.
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.
Please take a look.
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?
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.