mstate: basic service units

mstate: basic service units



https://code.launchpad.net/~aramh/juju-core/mstate-units-basic/+merge/112243

Requires: https://code.launchpad.net/~aramh/juju-core/state-mstate-doc-import1/+merge/112222

(do not edit description out of merge proposal)

[aram, 2012-06-26 22:45:13]

Please take a look.

[niemeyer, 2012-06-26 23:43:30]

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode64
mstate/service.go:64: func (s *Service) addUnit(principalName string) (unit
*Unit, err error) {
A unit is subordinate to another specific unit. It can't be a service name.

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode74
mstate/service.go:74: IsPrincipal: principalName == "",
We need to store the actual principal unit key, similar to how it's done in
state.

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode76
mstate/service.go:76: err = s.st.units.Insert(udoc)
Now, this time we'll have to stop and consider this for a moment. What if the
service is removed while we're doing this? What if the principal unit is removed
while we're attempting to add a subordinate to it?

Before committing to a solution, would you mind to bring up the conversation on
juju-dev so we can brainstorm a bit? It should take into account the possible
races, and if you have ideas about how to solve it simply, just mention them.
Otherwise, just the problem statement is fine, and we can brainstorm from there.

Please do the changes in this branch no matter what. If this is the only issue
it can go in and the race issue can be fixed in the next branch.

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode116
mstate/service.go:116: // RemoveUnit() removes a unit.
// RemovesUnit removes the given unit from s.

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode151
mstate/service.go:151: udocs := []unitDoc{}
s/udocs/docs/ is fine

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode156
mstate/service.go:156: for _, v := range udocs {
This is copying each document. I suggest logic like this instead:

for i := range docs {
    units = append(units, s.unit(&docs[i])
}

Then, reuse that s.unit method on other cases above.

https://codereview.appspot.com/6346044/diff/2001/mstate/state_test.go
File mstate/state_test.go (right):

https://codereview.appspot.com/6346044/diff/2001/mstate/state_test.go#newcode285
mstate/state_test.go:285: // TODO port subordinate unit logic from
state_test.TestAddUnit().
What's missing in this CL that means we can't have it?

[aram, 2012-06-27 22:21:41]

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode64
mstate/service.go:64: func (s *Service) addUnit(principalName string) (unit
*Unit, err error) {
On 2012/06/26 23:43:30, niemeyer wrote:
> A unit is subordinate to another specific unit. It can't be a service name.

I don't understand, principalName is the name of the principal unit, not of the
service. I use unit names as keys in the database instead of some opaque key
field because unit names are unique. Maybe the naming of the variable is not
right, the comment above certainly isn't. Fixed.

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode74
mstate/service.go:74: IsPrincipal: principalName == "",
On 2012/06/26 23:43:30, niemeyer wrote:
> We need to store the actual principal unit key, similar to how it's done in
> state.

Done.

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode116
mstate/service.go:116: // RemoveUnit() removes a unit.
On 2012/06/26 23:43:30, niemeyer wrote:
> // RemovesUnit removes the given unit from s.

Done.

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode151
mstate/service.go:151: udocs := []unitDoc{}
On 2012/06/26 23:43:30, niemeyer wrote:
> s/udocs/docs/ is fine

Done.

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode156
mstate/service.go:156: for _, v := range udocs {
On 2012/06/26 23:43:30, niemeyer wrote:
> This is copying each document. I suggest logic like this instead:
> 
> for i := range docs {
>     units = append(units, s.unit(&docs[i])
> }
> 
> Then, reuse that s.unit method on other cases above.

Done it a bit differently with a newUnit() function that's analogous to the
newCharm function.

https://codereview.appspot.com/6346044/diff/2001/mstate/state_test.go
File mstate/state_test.go (right):

https://codereview.appspot.com/6346044/diff/2001/mstate/state_test.go#newcode285
mstate/state_test.go:285: // TODO port subordinate unit logic from
state_test.TestAddUnit().
On 2012/06/26 23:43:30, niemeyer wrote:
> What's missing in this CL that means we can't have it?

AssignToMachine and AssignedMachineId were missing, not any more.

[aram, 2012-06-27 22:22:09]

Please take a look.

[niemeyer, 2012-06-28 07:28:12]

Thanks Aram, but please revert the unrelated code introduced since the last
review.

Then, LGTM just sorting out the trivial described.

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6346044/diff/2001/mstate/service.go#newcode64
mstate/service.go:64: func (s *Service) addUnit(principalName string) (unit
*Unit, err error) {
On 2012/06/27 22:21:41, aram wrote:
> On 2012/06/26 23:43:30, niemeyer wrote:
> > A unit is subordinate to another specific unit. It can't be a service name.
> 
> I don't understand, principalName is the name of the principal unit, not of
the
> service. I use unit names as keys in the database instead of some opaque key
> field because unit names are unique. Maybe the naming of the variable is not
> right, the comment above certainly isn't. Fixed.

My bad. Code looks fine, thanks.

https://codereview.appspot.com/6346044/diff/2001/mstate/state_test.go
File mstate/state_test.go (right):

https://codereview.appspot.com/6346044/diff/2001/mstate/state_test.go#newcode285
mstate/state_test.go:285: // TODO port subordinate unit logic from
state_test.TestAddUnit().
On 2012/06/27 22:21:41, aram wrote:
> On 2012/06/26 23:43:30, niemeyer wrote:
> > What's missing in this CL that means we can't have it?
> 
> AssignToMachine and AssignedMachineId were missing, not any more.

Sorry, I was just trying to understand really. Please drop the new code
introduced (including Units) and add the TODO back. This branch is good to go in
with the trivial details of this review sorted, but if we add these functions
now we'll need another round of reviews which will block the whole thing. Then,
please push the changes reverted into another CL so we can get them in.

https://codereview.appspot.com/6346044/diff/6002/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6346044/diff/6002/mstate/service.go#newcode140
mstate/service.go:140: return nil, errors.New("service name does not match
service")
"wrong service for unit" would be a bit better.

[aram, 2012-06-28 12:38:36]

Please take a look.

[aram, 2012-06-28 12:39:29]

*** Submitted:

mstate: basic service units

R=niemeyer
CC=
https://codereview.appspot.com/6346044