Merge lp://staging/~dimitern/juju-core/043-api-client-new-watchers into lp://staging/~juju/juju-core/trunk

Reviewers: mp+165603_code.launchpad.net,

Message:
Please take a look.

Description:
state/api: New client API watchers; api.State

Implemented two API client-side watchers:
* LifecycleWatcher
* EnvironConfigWatcher

Also introducing a top-level api.State object to
provide API access to state.* calls. It's only
accessible by agents.
Implemented methods:
* State.WatchMachines (using LifecycleWatcher)
* State.WatchEnvironConfig (using EnvironConfigWatcher)

https://code.launchpad.net/~dimitern/juju-core/043-api-client-new-watchers/+merge/165603

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M state/api/apiclient.go
   M state/api/params/params.go
   M state/apiserver/api_test.go
   M state/apiserver/apiserver.go

LGTM, only again one comment.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode599
state/api/apiclient.go:599: w.commonWatcher.init()
Hmm, doing it this way, with internal panics, init() also could be the
first instruction of commonLoop().

https://codereview.appspot.com/9714044/

LGTM with a few suggestions below.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode602
state/api/apiclient.go:602: // Watch calls Next internally at the
server-side, so we expect
this isn't quite right. how about:

// The first watch call returns the initial result
// value which we try to send immediately.
?

(and below)

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode659
state/api/apiclient.go:659: w.newResult = func() interface{} { return
new(params.EnvironConfigWatchResults) }
split body onto a new line for consistency with w.call below?

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode677
state/api/apiclient.go:677: // We have received changes, so send them
out.
this comment is a bit misleading - we're making them available to be
sent, not actually sending them.

i tried to think of a better comment, but honestly, i think the code is
clear enough without.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode681
state/api/apiclient.go:681: log.Errorf("state/api: error reading environ
config: %v", err)
s/config:/config from watcher:/ ?

https://codereview.appspot.com/9714044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/params/params.go#newcode154
state/api/params/params.go:154: // State.WatchServices(): id of the
created LifecycleWatcher and the
i find this a bit hard to parse.

how about something like this? (and similar for
EnvironConfigWatchResults)

// 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
}

https://codereview.appspot.com/9714044/diff/1/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/9714044/diff/1/state/apiserver/api_test.go#newcode1334
state/apiserver/api_test.go:1334: for i := 0; i < len(stMachines); i++ {
for i := range stMachines {

https://codereview.appspot.com/9714044/diff/1/state/apiserver/api_test.go#newcode1429
state/apiserver/api_test.go:1429: // Setup state - add entites to watch.
s/Setup/Set up/
s/entites/entities/

https://codereview.appspot.com/9714044/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/9714044/diff/1/state/apiserver/apiserver.go#newcode72
state/apiserver/apiserver.go:72: func (s *srvState) WatchMachines()
(params.LifecycleWatchResults, error) {
these two methods should be further down the file, perhaps just before
the srvMachine methods.

also, i'm not...

*** Submitted:

state/api: New client API watchers; api.State

Implemented two API client-side watchers:
* LifecycleWatcher
* EnvironConfigWatcher

Also introducing a top-level api.State object to
provide API access to state.* calls. It's only
accessible by agents.
Implemented methods:
* State.WatchMachines (using LifecycleWatcher)
* State.WatchEnvironConfig (using EnvironConfigWatcher)
These two can be called only by the environment manager
(for now, if we need we'll relax this restriction later).
Also, Machine.EnsureDead now can be called both by the
owning agent and the environment manager.

R=mue, rog
CC=
https://codereview.appspot.com/9714044

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode599
state/api/apiclient.go:599: w.commonWatcher.init()
On 2013/05/24 13:16:55, mue wrote:
> Hmm, doing it this way, with internal panics, init() also could be the
first
> instruction of commonLoop().

Calling init() in commonLoop() is not quite the same, because it's needs
to be called before the go routine spins up. This causes timeouts in
tests. I'll leave it as is if you don't mind.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode602
state/api/apiclient.go:602: // Watch calls Next internally at the
server-side, so we expect
On 2013/05/24 13:35:13, rog wrote:
> this isn't quite right. how about:

> // The first watch call returns the initial result
> // value which we try to send immediately.
> ?

> (and below)

Done.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode659
state/api/apiclient.go:659: w.newResult = func() interface{} { return
new(params.EnvironConfigWatchResults) }
On 2013/05/24 13:35:13, rog wrote:
> split body onto a new line for consistency with w.call below?

Done.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode677
state/api/apiclient.go:677: // We have received changes, so send them
out.
On 2013/05/24 13:35:13, rog wrote:
> this comment is a bit misleading - we're making them available to be
sent, not
> actually sending them.

> i tried to think of a better comment, but honestly, i think the code
is clear
> enough without.

Removed.

https://codereview.appspot.com/9714044/diff/1/state/api/apiclient.go#newcode681
state/api/apiclient.go:681: log.Errorf("state/api: error reading environ
config: %v", err)
On 2013/05/24 13:35:13, rog wrote:
> s/config:/config from watcher:/ ?

Done.

https://codereview.appspot.com/9714044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/9714044/diff/1/state/api/params/params.go#newcode154
state/api/params/params.go:154: // State.WatchServices(): id of the
created LifecycleWatcher and the
On 2013/05/24 13:35:13, rog wrote:
> i find this a bit hard to parse.

> how about something like this? (and similar for
EnvironConfigWatchResults)

> // 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 request...