state/apiserver: StringsWatcher common impl.

state/apiserver: StringsWatcher common impl.

Replaced srvLifecycleWatcher with srvStringsWatcher.
This will be the server-side implementation for any
watcher that returns a list of strings, like the
LifecycleWatcher, MinUnitsWatcher, UnitsWatcher, or
MachineUnitsWatcher.

At client side of the API there will be a single
StringsWatcher implementation of all these watchers,
thus greatly reducing code duplication (as in state).

Also, CleanupWatcher was made conformant with NotifyWatcher,
since it's the same interface. Workers were updated to
reflect the changes to the interface and a new api/interface.go
was introduced to keep the watcher's interfaces there.

https://code.launchpad.net/~dimitern/juju-core/066-apiserver-stringswatcher/+merge/173896

(do not edit description out of merge proposal)

[dimitern, 2013-07-10 09:54:53]

Please take a look.

[fwereade, 2013-07-10 10:00:37]

LGTM, but one quibble re description: I don't mind the client side having
distinct types so long as they're clearly thought through (ie there's no
justification for making client types that mirror the unexported concrete types
backing them in state).

So UnitsLifecycleWatcher would be ok, or StringsWatcher would be ok, but
LifecycleWatcher I think would not. Either way, there should only be one
concrete client-side type for StringsWatcher, there's no point duplicating all
that fiddly logic.

[jameinel, 2013-07-10 10:11:39]

LGTM

This looks like progress to me, so even if it isn't perfect better to land and
iterate than stall.

https://codereview.appspot.com/10944044/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (left):

https://codereview.appspot.com/10944044/diff/1/state/api/watcher/watcher.go#o...
state/api/watcher/watcher.go:193: func (w *LifecycleWatcher) loop() error {
I would like to think that this LifecycleWatcher would end up being just a
StringsWatcher as well.

[dimitern, 2013-07-10 10:45:25]

Please take a look.

https://codereview.appspot.com/10944044/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (left):

https://codereview.appspot.com/10944044/diff/1/state/api/watcher/watcher.go#o...
state/api/watcher/watcher.go:193: func (w *LifecycleWatcher) loop() error {
On 2013/07/10 10:11:39, jameinel wrote:
> I would like to think that this LifecycleWatcher would end up being just a
> StringsWatcher as well.
> 

Done.

[jameinel, 2013-07-10 10:52:13]

I'm a little unsure of what should be in 'params' and what should be in api. I
personally *like* api.NotifyWatcher better than params.NotifyWatcher.

I think I was running into problems with circular imports, but maybe that has
been sorted out.

LGTM

https://codereview.appspot.com/10944044/diff/6001/state/api/interface.go
File state/api/interface.go (right):

https://codereview.appspot.com/10944044/diff/6001/state/api/interface.go#newc...
state/api/interface.go:3: 
The other packages seem to call this file "interfaces.go".

tools/interfaces.go
state/apiserver/common/interfaces.go
state/api/common/interfaces.go
environs/interface.go ***

I'd probably go with interfaces (plural) myself, as there is more than one
interface here, right?

(It isn't the interface *of* the API, it is the interfaces *in* the API)

[fwereade, 2013-07-10 11:05:08]

LGTM, just one note: the workers that were using params.*Watcher should probably
really be using the state.*Watcher interface instead, because they are after all
still implemented in terms of state. Not really bothered, though, it'll all come
out the same in the end.

[dimitern, 2013-07-10 11:07:31]

https://codereview.appspot.com/10944044/diff/6001/state/api/interface.go
File state/api/interface.go (right):

https://codereview.appspot.com/10944044/diff/6001/state/api/interface.go#newc...
state/api/interface.go:3: 
On 2013/07/10 10:52:13, jameinel wrote:
> The other packages seem to call this file "interfaces.go".
> 
> tools/interfaces.go
> state/apiserver/common/interfaces.go
> state/api/common/interfaces.go
> environs/interface.go ***
> 
> I'd probably go with interfaces (plural) myself, as there is more than one
> interface here, right?
> 
> (It isn't the interface *of* the API, it is the interfaces *in* the API)

Renamed.

[dimitern, 2013-07-10 11:14:16]

Please take a look.