Add a watcher for the minUnits collection.

Add a watcher for the minUnits collection.

The watcher notifies about changes in the minUnits 
collection, i.e. it sends events when a service 
MinUnits is increased or a unit is destroyed.

https://code.launchpad.net/~frankban/juju-core/minimum-units-watcher/+merge/171741

(do not edit description out of merge proposal)

[frankban, 2013-06-27 09:07:50]

Please take a look.

[dimitern, 2013-06-27 14:54:48]

Looks reasonable, but I have a few suggestions.
Do you really need to return []string from Changes()? Can't you just return
struct{}{}, so it's just a notification?

https://codereview.appspot.com/10675043/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1632
state/state_test.go:1632: // Check no events.
// No events should occur.

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1638
state/state_test.go:1638: // Check one event.
// A single change should occur.

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1644
state/state_test.go:1644: // Check no events.
// Expect no changes.

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1662
state/state_test.go:1662: // Also avoid the unit to be removed.
s/to be removed/removal/ ?

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1674
state/state_test.go:1674: // Check one event, with the service name not
repeated.
please rephrase this comment it's ambiguous.

https://codereview.appspot.com/10675043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10675043/diff/1/state/watcher.go#newcode348
state/watcher.go:348: serviceNames []string, change watcher.Change) ([]string,
error) {
please don't do that, instead put it on a single line:
func (w *MinUnitsWatcher) merge(serviceNames []string, change watcher.Change)
([]string, error) {

https://codereview.appspot.com/10675043/diff/1/state/watcher.go#newcode376
state/watcher.go:376: case <-w.st.watcher.Dead():
you can probably skip this case, if you do this in the changes case below:
case changes, ok := <-ch:
  if !ok {
    return watcher.MustErr(w.st.watcher)
  }
  ...

[frankban, 2013-06-27 17:23:55]

On 2013/06/27 14:54:48, dimitern wrote:
> Looks reasonable, but I have a few suggestions.
> Do you really need to return []string from Changes()? Can't you just return
> struct{}{}, so it's just a notification?

In a future branch, we want to react to those notification, adding units
if required to services with a minimum number of units set.
For this reason, we want to know the service(s) associated with each
watcher event.

[frankban, 2013-06-27 17:31:50]

Please take a look.

https://codereview.appspot.com/10675043/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1632
state/state_test.go:1632: // Check no events.
On 2013/06/27 14:54:48, dimitern wrote:
> // No events should occur.

Done.

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1638
state/state_test.go:1638: // Check one event.
On 2013/06/27 14:54:48, dimitern wrote:
> // A single change should occur.

Done.

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1644
state/state_test.go:1644: // Check no events.
On 2013/06/27 14:54:48, dimitern wrote:
> // Expect no changes.

Done.

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1662
state/state_test.go:1662: // Also avoid the unit to be removed.
On 2013/06/27 14:54:48, dimitern wrote:
> s/to be removed/removal/ ?

Done.

https://codereview.appspot.com/10675043/diff/1/state/state_test.go#newcode1674
state/state_test.go:1674: // Check one event, with the service name not
repeated.
On 2013/06/27 14:54:48, dimitern wrote:
> please rephrase this comment it's ambiguous.

Done.

https://codereview.appspot.com/10675043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10675043/diff/1/state/watcher.go#newcode348
state/watcher.go:348: serviceNames []string, change watcher.Change) ([]string,
error) {
On 2013/06/27 14:54:48, dimitern wrote:
> please don't do that, instead put it on a single line:
> func (w *MinUnitsWatcher) merge(serviceNames []string, change watcher.Change)
> ([]string, error) {

Done, but I am curious about the reason for this request.

[fwereade, 2013-06-28 10:48:58]

Looking very nice, just a couple of suggestions.

https://codereview.appspot.com/10675043/diff/8001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10675043/diff/8001/state/state_test.go#newcode...
state/state_test.go:1624: wordpressName := wordpress.Name()
blank line please

https://codereview.appspot.com/10675043/diff/8001/state/state_test.go#newcode...
state/state_test.go:1638: // No events should occur.
That's not right ;p.

https://codereview.appspot.com/10675043/diff/8001/state/state_test.go#newcode...
state/state_test.go:1641: // Decrease minimum units for a service.
FWIW, I've found it very helpful to put both comments together at the top of
these blocks.

// Decrease minimum units of a service; check no changes.

https://codereview.appspot.com/10675043/diff/8001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10675043/diff/8001/state/watcher.go#newcode351
state/watcher.go:351: delete(w.known, serviceName)
I think we can remove from serviceNames as well? If it's not there we certainly
don't need to mention it again, even if we thought we were meant to a few ms
ago.

BTW, you can determine this situation without a DB hit by checking the
change.Revno -- it'll be -1 if the change represents a removal.

https://codereview.appspot.com/10675043/diff/8001/state/watcher.go#newcode358
state/watcher.go:358: if (!known || doc.Revno > revno) &&
!hasString(serviceNames, serviceName) {
Probably best to use a (juju-core/utils/)set.Strings to keep track of these
things. Looks like someone started to change this file over to use that but
didn't finish :).

[frankban, 2013-06-28 14:18:09]

Please take a look.

https://codereview.appspot.com/10675043/diff/8001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10675043/diff/8001/state/state_test.go#newcode...
state/state_test.go:1624: wordpressName := wordpress.Name()
On 2013/06/28 10:48:58, fwereade wrote:
> blank line please

Done.

https://codereview.appspot.com/10675043/diff/8001/state/state_test.go#newcode...
state/state_test.go:1638: // No events should occur.
On 2013/06/28 10:48:58, fwereade wrote:
> That's not right ;p.

Yeah...

https://codereview.appspot.com/10675043/diff/8001/state/state_test.go#newcode...
state/state_test.go:1641: // Decrease minimum units for a service.
On 2013/06/28 10:48:58, fwereade wrote:
> FWIW, I've found it very helpful to put both comments together at the top of
> these blocks.
> 
> // Decrease minimum units of a service; check no changes.

Done.

[frankban, 2013-06-28 14:23:52]

https://codereview.appspot.com/10675043/diff/8001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10675043/diff/8001/state/watcher.go#newcode351
state/watcher.go:351: delete(w.known, serviceName)
On 2013/06/28 10:48:58, fwereade wrote:
> I think we can remove from serviceNames as well? If it's not there we
certainly
> don't need to mention it again, even if we thought we were meant to a few ms
> ago.

Right, done.

> 
> BTW, you can determine this situation without a DB hit by checking the
> change.Revno -- it'll be -1 if the change represents a removal.

Good to know. Fixed, thank you.

https://codereview.appspot.com/10675043/diff/8001/state/watcher.go#newcode358
state/watcher.go:358: if (!known || doc.Revno > revno) &&
!hasString(serviceNames, serviceName) {
On 2013/06/28 10:48:58, fwereade wrote:
> Probably best to use a (juju-core/utils/)set.Strings to keep track of these
> things. Looks like someone started to change this file over to use that but
> didn't finish :).

Really nice, fixed.

[fwereade, 2013-06-28 14:30:08]

Lovely, LGTM

[dimitern, 2013-06-28 16:14:50]

LGTM with a minor grumble re a previous suggestion.

https://codereview.appspot.com/10675043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10675043/diff/1/state/watcher.go#newcode376
state/watcher.go:376: case <-w.st.watcher.Dead():
On 2013/06/27 14:54:48, dimitern wrote:
> you can probably skip this case, if you do this in the changes case below:
> case changes, ok := <-ch:
>   if !ok {
>     return watcher.MustErr(w.st.watcher)
>   }
>   ...

Haven't see this done, at least explain why you think it's better this way?

[frankban, 2013-06-28 16:51:59]

Please take a look.

https://codereview.appspot.com/10675043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10675043/diff/1/state/watcher.go#newcode376
state/watcher.go:376: case <-w.st.watcher.Dead():
On 2013/06/28 16:14:50, dimitern wrote:

> Haven't see this done, at least explain why you think it's better this way?

Sorry, missed it. Fixed now as you suggested, thank you!