Merge lp:~dimitern/juju-core/066-apiserver-stringswatcher into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

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://codereview.appspot.com/10944044/

R=fwereade, fwereade, jameinel, jameinel

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://codereview.appspot.com/10944044/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+173896_code.launchpad.net,

Message:
Please take a look.

Description:
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 concrete
implementations of each watcher still.

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10944044/

Affected files:
   A [revision details]
   M state/api/params/internal.go
   M state/api/watcher/watcher.go
   M state/apiserver/root.go
   M state/apiserver/watcher.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130709173747-vea1m7nxrla81n4o
+New revision:
<email address hidden>

Index: state/apiserver/root.go
=== modified file 'state/apiserver/root.go'
--- state/apiserver/root.go 2013-07-09 16:29:17 +0000
+++ state/apiserver/root.go 2013-07-10 09:49:15 +0000
@@ -110,11 +110,10 @@
   }, nil
  }

-// LifecycleWatcher returns an object that provides
-// API access to methods on a state.LifecycleWatcher.
-// Each client has its own current set of watchers, stored
-// in r.resources.
-func (r *srvRoot) LifecycleWatcher(id string) (*srvLifecycleWatcher,
error) {
+// StringsWatcher returns an object that provides API access to
+// methods on a state.StringsWatcher. Each client has its own
+// current set of watchers, stored in r.resources.
+func (r *srvRoot) StringsWatcher(id string) (*srvStringsWatcher, error) {
   if err := r.requireAgent(); err != nil {
    return nil, err
   }
@@ -122,7 +121,7 @@
   if !ok {
    return nil, common.ErrUnknownWatcher
   }
- return &srvLifecycleWatcher{
+ return &srvStringsWatcher{
    watcher: watcher,
    id: id,
    resources: r.resources,

Index: state/apiserver/watcher.go
=== modified file 'state/apiserver/watcher.go'
--- state/apiserver/watcher.go 2013-07-09 16:29:17 +0000
+++ state/apiserver/watcher.go 2013-07-10 09:49:15 +0000
@@ -52,31 +52,31 @@
   return w.resources.Stop(w.id)
  }

-// srvLifecycleWatcher notifies about lifecycle changes for all
-// entities of a given kind. See state.lifecycleWatcher.
-type srvLifecycleWatcher struct {
+// srvStringsWatcher notifies about changes for all entities of a
+// given kind, sending the changes as a list of strings.
+type srvStringsWatcher struct {
   watcher state.StringsWatcher
   id string
   resources *common.Resources
  }

-// Next returns when a change has occured to the lifecycle of an
-// entity of the collection being watched since the most recent call
-// to Next or the Watch call that created the srvLifecycleWatcher.
-func (w *srvLifecycleWatcher) Next() (params.LifecycleWatchResults, error)
{
+// Next returns when a change has occured to an entity of the
+// collection being watched since...

[William Reade (fwereade)]

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.

https://codereview.appspot.com/10944044/

[John A Meinel (jameinel)]

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#oldcode193
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.

https://codereview.appspot.com/10944044/

[Dimiter Naydenov (dimitern)]

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#oldcode193
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.

https://codereview.appspot.com/10944044/

[John A Meinel (jameinel)]

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#newcode3
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)

https://codereview.appspot.com/10944044/

[William Reade (fwereade)]

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.

https://codereview.appspot.com/10944044/

[Dimiter Naydenov (dimitern)]

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#newcode3
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.

https://codereview.appspot.com/10944044/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/10944044/

[Go Bot (go-bot)]

No approved revision specified.

=== added file 'state/api/interfaces.go'
--- state/api/interfaces.go	1970-01-01 00:00:00 +0000
+++ state/api/interfaces.go	2013-07-10 11:15:34 +0000
@@ -0,0 +1,20 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package api
+
+// NotifyWatcher will send events when something changes.
+// It does not send content for those changes.
+type NotifyWatcher interface {
+	Changes() <-chan struct{}
+	Stop() error
+	Err() error
+}
+
+// StringsWatcher will send events when something changes.
+// The content for the changes is a list of strings.
+type StringsWatcher interface {
+	Changes() <-chan []string
+	Stop() error
+	Err() error
+}

=== modified file 'state/api/params/internal.go'
--- state/api/params/internal.go	2013-07-08 21:49:45 +0000
+++ state/api/params/internal.go	2013-07-10 11:15:34 +0000
@@ -103,15 +103,7 @@
 	Password string
 }
 
-// A NotifyWatcher will send events when something changes.
-// It does not send content for those changes.
-type NotifyWatcher interface {
-	Changes() <-chan struct{}
-	Stop() error
-	Err() error
-}
-
-// NotifyWatchResult holds an NotifyWatcher id and an error (if any).
+// NotifyWatchResult holds a NotifyWatcher id and an error (if any).
 type NotifyWatchResult struct {
 	NotifyWatcherId string
 	Error           *Error
@@ -123,19 +115,10 @@
 	Results []NotifyWatchResult
 }
 
-// LifecycleWatchResults holds the results of API calls
-// that watch the lifecycle of a set of objects.
-// It is used both for the initial Watch request
-// and for subsequent Next requests.
-type LifecycleWatchResults struct {
-	// LifeCycleWatcherId holds the id of the newly
-	// created watcher. It will be empty for a Next
-	// request.
-	LifecycleWatcherId string
-
-	// Ids holds the list of entity ids.
-	// For a Watch request, it holds all entity ids being
-	// watched; for a Next request, it holds the ids of those
-	// that have changed.
-	Ids []string
+// StringsWatchResult holds a StringsWatcher id, changes and an error
+// (if any).
+type StringsWatchResult struct {
+	StringsWatcherId string
+	Changes          []string
+	Error            *Error
 }

=== modified file 'state/api/watcher/watcher.go'
--- state/api/watcher/watcher.go	2013-07-08 10:44:10 +0000
+++ state/api/watcher/watcher.go	2013-07-10 11:15:34 +0000
@@ -5,6 +5,7 @@
 
 import (
 	"launchpad.net/juju-core/log"
+	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/common"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/tomb"
@@ -108,6 +109,8 @@
 	return w.tomb.Err()
 }
 
+// notifyWatcher will send events when something changes.
+// It does not send content for those changes.
 type notifyWatcher struct {
 	commonWatcher
 	caller          common.Caller
@@ -117,7 +120,7 @@
 
 // If an API call returns a NotifyWatchResult, you can use this to turn it into
 // a local Watcher.
-func NewNotifyWatcher(caller common.Caller, result params.NotifyWatchResult) params.NotifyWatcher {
+func NewNotifyWatcher(caller common.Caller, result params.NotifyWatchResult) api.NotifyWatcher {
 	w := &notifyWatcher{
 		caller:          caller,
 		notifyWatcherId: result.NotifyWatcherId,
@@ -169,15 +172,17 @@
 	return w.out
 }
 
-type LifecycleWatcher struct {
+// stringsWatcher will send events when something changes.
+// The content of the changes is a list of strings.
+type stringsWatcher struct {
 	commonWatcher
 	caller    common.Caller
 	watchCall string
 	out       chan []string
 }
 
-func newLifecycleWatcher(caller common.Caller, watchCall string) *LifecycleWatcher {
-	w := &LifecycleWatcher{
+func NewStringsWatcher(caller common.Caller, watchCall string) api.StringsWatcher {
+	w := &stringsWatcher{
 		caller:    caller,
 		watchCall: watchCall,
 		out:       make(chan []string),
@@ -190,15 +195,15 @@
 	return w
 }
 
-func (w *LifecycleWatcher) loop() error {
-	var result params.LifecycleWatchResults
+func (w *stringsWatcher) loop() error {
+	var result params.StringsWatchResult
 	if err := w.caller.Call("State", "", w.watchCall, nil, &result); err != nil {
 		return err
 	}
-	changes := result.Ids
-	w.newResult = func() interface{} { return new(params.LifecycleWatchResults) }
+	changes := result.Changes
+	w.newResult = func() interface{} { return new(params.StringsWatchResult) }
 	w.call = func(request string, newResult interface{}) error {
-		return w.caller.Call("LifecycleWatcher", result.LifecycleWatcherId, request, nil, newResult)
+		return w.caller.Call("StringsWatcher", result.StringsWatcherId, request, nil, newResult)
 	}
 	w.commonWatcher.init()
 	go w.commonLoop()
@@ -215,7 +220,7 @@
 				return nil
 			}
 			// We have received changes, so send them out.
-			changes = data.(*params.LifecycleWatchResults).Ids
+			changes = data.(*params.StringsWatchResult).Changes
 			out = w.out
 		case out <- changes:
 			// Wait until we have new changes to send.
@@ -225,8 +230,8 @@
 	panic("unreachable")
 }
 
-// Changes returns a channel that receives a list of ids of watched
-// entites whose lifecycle has changed.
-func (w *LifecycleWatcher) Changes() <-chan []string {
+// Changes returns a channel that receives a list of strings of watched
+// entites with changes.
+func (w *stringsWatcher) Changes() <-chan []string {
 	return w.out
 }

=== modified file 'state/apiserver/root.go'
--- state/apiserver/root.go	2013-07-09 16:29:17 +0000
+++ state/apiserver/root.go	2013-07-10 11:15:34 +0000
@@ -110,11 +110,10 @@
 	}, nil
 }
 
-// LifecycleWatcher returns an object that provides
-// API access to methods on a state.LifecycleWatcher.
-// Each client has its own current set of watchers, stored
-// in r.resources.
-func (r *srvRoot) LifecycleWatcher(id string) (*srvLifecycleWatcher, error) {
+// StringsWatcher returns an object that provides API access to
+// methods on a state.StringsWatcher.  Each client has its own
+// current set of watchers, stored in r.resources.
+func (r *srvRoot) StringsWatcher(id string) (*srvStringsWatcher, error) {
 	if err := r.requireAgent(); err != nil {
 		return nil, err
 	}
@@ -122,7 +121,7 @@
 	if !ok {
 		return nil, common.ErrUnknownWatcher
 	}
-	return &srvLifecycleWatcher{
+	return &srvStringsWatcher{
 		watcher:   watcher,
 		id:        id,
 		resources: r.resources,

=== modified file 'state/apiserver/watcher.go'
--- state/apiserver/watcher.go	2013-07-09 16:29:17 +0000
+++ state/apiserver/watcher.go	2013-07-10 11:15:34 +0000
@@ -52,31 +52,31 @@
 	return w.resources.Stop(w.id)
 }
 
-// srvLifecycleWatcher notifies about lifecycle changes for all
-// entities of a given kind. See state.lifecycleWatcher.
-type srvLifecycleWatcher struct {
+// srvStringsWatcher notifies about changes for all entities of a
+// given kind, sending the changes as a list of strings.
+type srvStringsWatcher struct {
 	watcher   state.StringsWatcher
 	id        string
 	resources *common.Resources
 }
 
-// Next returns when a change has occured to the lifecycle of an
-// entity of the collection being watched since the most recent call
-// to Next or the Watch call that created the srvLifecycleWatcher.
-func (w *srvLifecycleWatcher) Next() (params.LifecycleWatchResults, error) {
+// Next returns when a change has occured to an entity of the
+// collection being watched since the most recent call to Next
+// or the Watch call that created the srvStringsWatcher.
+func (w *srvStringsWatcher) Next() (params.StringsWatchResult, error) {
 	if changes, ok := <-w.watcher.Changes(); ok {
-		return params.LifecycleWatchResults{
-			Ids: changes,
+		return params.StringsWatchResult{
+			Changes: changes,
 		}, nil
 	}
 	err := w.watcher.Err()
 	if err == nil {
 		err = common.ErrStoppedWatcher
 	}
-	return params.LifecycleWatchResults{}, err
+	return params.StringsWatchResult{}, err
 }
 
 // Stop stops the watcher.
-func (w *srvLifecycleWatcher) Stop() error {
+func (w *srvStringsWatcher) Stop() error {
 	return w.resources.Stop(w.id)
 }

=== modified file 'state/watcher.go'
--- state/watcher.go	2013-07-09 16:29:17 +0000
+++ state/watcher.go	2013-07-10 11:15:34 +0000
@@ -1226,19 +1226,19 @@
 	panic("unreachable")
 }
 
-// CleanupWatcher notifies of changes in the cleanups collection.
-type CleanupWatcher struct {
+// cleanupWatcher notifies of changes in the cleanups collection.
+type cleanupWatcher struct {
 	commonWatcher
 	out chan struct{}
 }
 
 // WatchCleanups starts and returns a CleanupWatcher.
-func (st *State) WatchCleanups() *CleanupWatcher {
+func (st *State) WatchCleanups() NotifyWatcher {
 	return newCleanupWatcher(st)
 }
 
-func newCleanupWatcher(st *State) *CleanupWatcher {
-	w := &CleanupWatcher{
+func newCleanupWatcher(st *State) NotifyWatcher {
+	w := &cleanupWatcher{
 		commonWatcher: commonWatcher{st: st},
 		out:           make(chan struct{}),
 	}
@@ -1251,11 +1251,11 @@
 }
 
 // Changes returns the event channel for w.
-func (w *CleanupWatcher) Changes() <-chan struct{} {
+func (w *cleanupWatcher) Changes() <-chan struct{} {
 	return w.out
 }
 
-func (w *CleanupWatcher) loop() (err error) {
+func (w *cleanupWatcher) loop() (err error) {
 	in := make(chan watcher.Change)
 
 	w.st.watcher.WatchCollection(w.st.cleanups.Name, in)

=== modified file 'worker/cleaner/cleaner.go'
--- worker/cleaner/cleaner.go	2013-07-08 12:29:48 +0000
+++ worker/cleaner/cleaner.go	2013-07-10 11:15:34 +0000
@@ -8,7 +8,7 @@
 
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/state"
-	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/worker"
 )
 
@@ -27,7 +27,7 @@
 	return fmt.Sprintf("cleaner")
 }
 
-func (c *Cleaner) SetUp() (params.NotifyWatcher, error) {
+func (c *Cleaner) SetUp() (api.NotifyWatcher, error) {
 	return c.st.WatchCleanups(), nil
 }
 

=== modified file 'worker/notifyworker.go'
--- worker/notifyworker.go	2013-07-08 18:50:58 +0000
+++ worker/notifyworker.go	2013-07-10 11:15:34 +0000
@@ -6,7 +6,7 @@
 import (
 	"launchpad.net/tomb"
 
-	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/watcher"
 )
 
@@ -47,7 +47,7 @@
 	// SetUp starts the handler, this should create the watcher we will be
 	// waiting on for more events. SetUp can return a Watcher even if there
 	// is an error, and NotifyWorker will make sure to stop the Watcher.
-	SetUp() (params.NotifyWatcher, error)
+	SetUp() (api.NotifyWatcher, error)
 
 	// TearDown should cleanup any resources that are left around
 	TearDown() error
@@ -106,7 +106,7 @@
 }
 
 func (nw *notifyWorker) loop() error {
-	var w params.NotifyWatcher
+	var w api.NotifyWatcher
 	var err error
 	defer handlerTearDown(nw.handler, &nw.tomb)
 	if w, err = nw.handler.SetUp(); err != nil {

=== modified file 'worker/notifyworker_test.go'
--- worker/notifyworker_test.go	2013-07-09 08:40:22 +0000
+++ worker/notifyworker_test.go	2013-07-10 11:15:34 +0000
@@ -11,7 +11,7 @@
 	gc "launchpad.net/gocheck"
 	"launchpad.net/tomb"
 
-	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/watcher"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
@@ -56,7 +56,7 @@
 	description   string
 }
 
-func (a *actionsHandler) SetUp() (params.NotifyWatcher, error) {
+func (a *actionsHandler) SetUp() (api.NotifyWatcher, error) {
 	a.mu.Lock()
 	defer a.mu.Unlock()
 	a.actions = append(a.actions, "setup")