Code review comment for lp:~rogpeppe/juju-core/479-desired-peer-group

Revision history for this message
John A Meinel (jameinel) wrote :

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.go#newcode32
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.go#newcode58
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.go#newcode66
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.

https://codereview.appspot.com/49920045/

« Back to merge proposal