Code review - Issue 6248076: add state.AssignmentPolicy type and state.AssignUnit funchttps://codereview.appspot.com/2012-06-06T10:52:46+00:00rietveld
Message from unknown
2012-05-31T13:31:54+00:00fwereadeurn:md5:d53adc42c48de79a9779b7739a37c95a
Message from fwereade@gmail.com
2012-05-31T13:32:00+00:00fwereadeurn:md5:c2a7575ac2f65174ea72563397287ac9
Please take a look.
Message from n13m3y3r@gmail.com
2012-06-05T19:58:07+00:00niemeyerurn:md5:9771627d1d770a11da70631daced0707
Implementation looks nice. I have a couple of API cleanup suggestions for you to consider. Please let me know what you think.
https://codereview.appspot.com/6248076/diff/1/state/unit.go
File state/unit.go (right):
https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode26
state/unit.go:26: type PlacementPolicy string
Can we name this as AssignmentPolicy? I'd also be equally happy to go to the other direction and name methods as PlaceInMachine etc.
https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode30
state/unit.go:30: PlaceUnassigned PlacementPolicy = "unassigned"
It'd be nice to have a sentence on top of each explaining their meaning.
https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode241
state/unit.go:241: func (u *Unit) Place(policy PlacementPolicy) (err error) {
It's awkward to have this as a method of the unit. Not only is it using the unit just through its API, but it's also digging through and *creating machines* silently.
IMO, this is better suited for a utility function such as:
func AssignUnit(st *State, u *Unit, policy AssignmentPolicy)
(or Place/Placement, depending on which side you prefer)
It'd be useful as well to document the fact it not only assigns the unit but may also create a brand new machine to accomplish the task it was given.
https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode250
state/unit.go:250: case noUnusedMachines:
if err != noUnusedMachines {
return err
}
Message from unknown
2012-06-06T10:18:31+00:00fwereadeurn:md5:de6ef4af03f9d4d22907bc7dbfc64ce3
Message from fwereade@gmail.com
2012-06-06T10:18:35+00:00fwereadeurn:md5:7c9c7b2cd0822d8c5a6f5b3ba0e8e2ca
Please take a look.
https://codereview.appspot.com/6248076/diff/1/state/unit.go
File state/unit.go (right):
https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode26
state/unit.go:26: type PlacementPolicy string
On 2012/06/05 19:58:08, niemeyer wrote:
> Can we name this as AssignmentPolicy? I'd also be equally happy to go to the
> other direction and name methods as PlaceInMachine etc.
Assignment SGTM
https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode30
state/unit.go:30: PlaceUnassigned PlacementPolicy = "unassigned"
On 2012/06/05 19:58:08, niemeyer wrote:
> It'd be nice to have a sentence on top of each explaining their meaning.
Done.
https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode241
state/unit.go:241: func (u *Unit) Place(policy PlacementPolicy) (err error) {
On 2012/06/05 19:58:08, niemeyer wrote:
> It's awkward to have this as a method of the unit. Not only is it using the unit
> just through its API, but it's also digging through and *creating machines*
> silently.
>
> IMO, this is better suited for a utility function such as:
>
> func AssignUnit(st *State, u *Unit, policy AssignmentPolicy)
>
> (or Place/Placement, depending on which side you prefer)
>
> It'd be useful as well to document the fact it not only assigns the unit but may
> also create a brand new machine to accomplish the task it was given.
Done.
https://codereview.appspot.com/6248076/diff/1/state/unit.go#newcode250
state/unit.go:250: case noUnusedMachines:
On 2012/06/05 19:58:08, niemeyer wrote:
> if err != noUnusedMachines {
> return err
> }
D'oh! Done.
Message from n13m3y3r@gmail.com
2012-06-06T10:40:04+00:00niemeyerurn:md5:f4e57389c5ae2f932e0507ed9904a5d4
LGTM, thanks. It'd be nice to move the helper function back where it was (or around where it was, anyway).
https://codereview.appspot.com/6248076/diff/5001/state/state_test.go
File state/state_test.go (right):
https://codereview.appspot.com/6248076/diff/5001/state/state_test.go#newcode1062
state/state_test.go:1062: func (s *StateSuite) TestAddRelation(c *C) {
This shouldn't be here, I suspect.
https://codereview.appspot.com/6248076/diff/5001/state/util.go
File state/util.go (right):
https://codereview.appspot.com/6248076/diff/5001/state/util.go#newcode26
state/util.go:26: func AssignUnit(st *State, u *Unit, policy AssignmentPolicy) (err error) {
It's fine to have this as a function in the place where it was. It's a much better spot than here, as it had all the logic surrounding it just a glance away.
Message from unknown
2012-06-06T10:52:08+00:00fwereadeurn:md5:4b58e02564a7cf53e95c47911e6e071e
Message from fwereade@gmail.com
2012-06-06T10:52:46+00:00fwereadeurn:md5:511327d3e3208878e23281da80f67fa7
*** Submitted:
add state.AssignmentPolicy type and state.AssignUnit func
R=niemeyer
CC=
https://codereview.appspot.com/6248076
https://codereview.appspot.com/6248076/diff/5001/state/state_test.go
File state/state_test.go (right):
https://codereview.appspot.com/6248076/diff/5001/state/state_test.go#newcode1062
state/state_test.go:1062: func (s *StateSuite) TestAddRelation(c *C) {
On 2012/06/06 10:40:04, niemeyer wrote:
> This shouldn't be here, I suspect.
Ignored per IRC :).
https://codereview.appspot.com/6248076/diff/5001/state/util.go
File state/util.go (right):
https://codereview.appspot.com/6248076/diff/5001/state/util.go#newcode26
state/util.go:26: func AssignUnit(st *State, u *Unit, policy AssignmentPolicy) (err error) {
On 2012/06/06 10:40:04, niemeyer wrote:
> It's fine to have this as a function in the place where it was. It's a much
> better spot than here, as it had all the logic surrounding it just a glance
> away.
Done.