state/api: implement machine watching

state/api: implement machine watching

This watcher is simpler than some because no
information is sent with each change notification.
It shows the basic pattern though.

https://code.launchpad.net/~rogpeppe/juju-core/210-api-first-watcher/+merge/149646

(do not edit description out of merge proposal)

[rog, 2013-02-20 17:50:53]

Please take a look.

[dimitern, 2013-02-21 11:23:29]

LGTM, with some trivial comments.

https://codereview.appspot.com/7390043/diff/2001/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7390043/diff/2001/state/api/api_test.go#newcod...
state/api/api_test.go:526: case <-time.After(20 * time.Millisecond):
Is this enough time to check no changes arrive on both watchers?

https://codereview.appspot.com/7390043/diff/2001/state/api/api_test.go#newcod...
state/api/api_test.go:640: err:  &state.NotAssignedError{&state.Unit{}}, // too
sleazy?!
indeed :)

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

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:43: err := c.st.call("Client", "", "Status", nil, &s)
if err := ..; err != nil { .. } ?

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:165: if err != nil {
as above

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:192: // Note that because the change notification
This sentence seems a bit incomplete.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:196: err = nil
Why is this not an error?

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go#newco...
state/api/apiserver.go:160: func (r *srvRoot) EntityWatcher(id string)
(srvEntityWatcher, error) {
Comment here please?

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go#newco...
state/api/apiserver.go:259: // Catch expected common case of mispelled
why misspelled? it only checks for empty one.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go#newco...
state/api/apiserver.go:391: Err() error
Is there a special reason no to implement Error() instead? Seems useful to be
able to handle watchers as errors in some cases.

https://codereview.appspot.com/7390043/diff/2001/state/api/error.go
File state/api/error.go (right):

https://codereview.appspot.com/7390043/diff/2001/state/api/error.go#newcode39
state/api/error.go:39: state.ErrExcessiveContention: CodeExcessiveContention,
I like this error :)

[fwereade, 2013-02-25 09:03:43]

This may be monday morning speaking, but I'm afraid I need some more explanation
of this.

https://codereview.appspot.com/7390043/diff/2001/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7390043/diff/2001/state/api/api_test.go#newcod...
state/api/api_test.go:554: case <-time.After(10 * time.Second):
500ms should be enough to time out, surely?

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

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:43: err := c.st.call("Client", "", "Status", nil, &s)
On 2013/02/21 11:23:29, dimitern wrote:
> if err := ..; err != nil { .. } ?
+1

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:172: // request to the server, which will remove the
watcher
Expand on this. ISTM that we block forever, waiting for the result of the last
call to Next, and have no way to interrupt a Next that's in progress.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:175: // has been stopped, we'll get a CodeNotFound error;
Who's stopping the watcher if not us? Why would we call next if we've stopped
the watcher?

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:190: return nil
tomb.ErrDying

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:195: if code := ErrCode(err); code == CodeStopped || code
== CodeNotFound {
I don't see how the Stop call gets sent when we're inside a Next call.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:196: err = nil
On 2013/02/21 11:23:29, dimitern wrote:
> Why is this not an error?
+1

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go#newco...
state/api/apiserver.go:160: func (r *srvRoot) EntityWatcher(id string)
(srvEntityWatcher, error) {
On 2013/02/21 11:23:29, dimitern wrote:
> Comment here please?
+1

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go#newco...
state/api/apiserver.go:391: Err() error
On 2013/02/21 11:23:29, dimitern wrote:
> Is there a special reason no to implement Error() instead? Seems useful to be
> able to handle watchers as errors in some cases.

Eww! Please don't do that. They're totally different things and no good will
come of conflating them IMO.

[rog, 2013-02-25 10:49:59]

Please take a look.

https://codereview.appspot.com/7390043/diff/2001/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7390043/diff/2001/state/api/api_test.go#newcod...
state/api/api_test.go:526: case <-time.After(20 * time.Millisecond):
On 2013/02/21 11:23:29, dimitern wrote:
> Is this enough time to check no changes arrive on both watchers?

no time is long enough, but this at least will check that
the watcher isn't sending changes continuously.

https://codereview.appspot.com/7390043/diff/2001/state/api/api_test.go#newcod...
state/api/api_test.go:554: case <-time.After(10 * time.Second):
On 2013/02/25 09:03:43, fwereade wrote:
> 500ms should be enough to time out, surely?

unfortunately not, as StartSync doesn't make a difference, so the poll interval
is 5 seconds. i wonder whether i should provide a call to StartSync in the API
for testing purposes. or perhaps you can think of another nice solution? (one
possibility that occurs to me is to make StartSync global)

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

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:43: err := c.st.call("Client", "", "Status", nil, &s)
On 2013/02/25 09:03:43, fwereade wrote:
> On 2013/02/21 11:23:29, dimitern wrote:
> > if err := ..; err != nil { .. } ?
> +1

Done.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:165: if err != nil {
On 2013/02/21 11:23:29, dimitern wrote:
> as above

Done.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:172: // request to the server, which will remove the
watcher
On 2013/02/25 09:03:43, fwereade wrote:
> Expand on this. ISTM that we block forever, waiting for the result of the last
> call to Next, and have no way to interrupt a Next that's in progress.

in EntityWatcher.Stop we kill the EntityWatcher's tomb, which triggers the below
Stop request to be sent, which causes the remote watcher to be stopped, which
interrupts the Next request, which then returns with a CodeStopped error.

but that's just saying the same as this comment. perhaps there's something i'm
not saying right.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:175: // has been stopped, we'll get a CodeNotFound error;
On 2013/02/25 09:03:43, fwereade wrote:
> Who's stopping the watcher if not us? Why would we call next if we've stopped
> the watcher?

We *could* arrange things so that we don't call Next if we've just called Stop,
but it would be awkward (we'd have to use w.st.client.Go and a mutex) - the
logic here is simpler IMHO and the efficiency gain is likely to be unmeasurable
(what's the likelihood of us calling EntityWatcher.Stop in the moment between
sending a notification on w.out and making the Next request?)

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:190: return nil
On 2013/02/25 09:03:43, fwereade wrote:
> tomb.ErrDying

Done.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:192: // Note that because the change notification
On 2013/02/21 11:23:29, dimitern wrote:
> This sentence seems a bit incomplete.

Done.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:195: if code := ErrCode(err); code == CodeStopped || code
== CodeNotFound {
On 2013/02/25 09:03:43, fwereade wrote:
> I don't see how the Stop call gets sent when we're inside a Next call.

it's triggered by the tomb dying inside the goroutine above.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiclient.go#newco...
state/api/apiclient.go:196: err = nil
On 2013/02/21 11:23:29, dimitern wrote:
> Why is this not an error?

see the comment above. either of these errors is to be expected when a watcher
has been stopped.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go#newco...
state/api/apiserver.go:160: func (r *srvRoot) EntityWatcher(id string)
(srvEntityWatcher, error) {
On 2013/02/25 09:03:43, fwereade wrote:
> On 2013/02/21 11:23:29, dimitern wrote:
> > Comment here please?
> +1

done. added comments to some other methods too.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go#newco...
state/api/apiserver.go:259: // Catch expected common case of mispelled
On 2013/02/21 11:23:29, dimitern wrote:
> why misspelled? it only checks for empty one.

if the caller sends a message with a field named "Passwd", we'll see it here as
an empty password field.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go#newco...
state/api/apiserver.go:391: Err() error
On 2013/02/21 11:23:29, dimitern wrote:
> Is there a special reason no to implement Error() instead? Seems useful to be
> able to handle watchers as errors in some cases.

this interface reflects the existing interface common to all state watcher
implementations - i'm not implementing it here.

[fwereade, 2013-02-25 11:24:13]

Sorry for the confusion; looking good, but I'll wait for the dropped-connection
handling.

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7390043/diff/2001/state/api/apiserver.go#newco...
state/api/apiserver.go:160: func (r *srvRoot) EntityWatcher(id string)
(srvEntityWatcher, error) {
On 2013/02/25 10:50:00, rog wrote:
> On 2013/02/25 09:03:43, fwereade wrote:
> > On 2013/02/21 11:23:29, dimitern wrote:
> > > Comment here please?
> > +1
> 
> done. added comments to some other methods too.

<3

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

https://codereview.appspot.com/7390043/diff/17001/state/api/apiclient.go#newc...
state/api/apiclient.go:178: go func() {
I think I might have been less confused if the explanation had been inside the
goroutine here. Just a thought.

https://codereview.appspot.com/7390043/diff/17001/state/api/apiclient.go#newc...
state/api/apiclient.go:193: // on w.out.
I think this comment is kinda redundant, but YMMV.

[rog, 2013-02-26 11:22:39]

Please take a look.

[rog, 2013-02-27 10:46:53]

Please take a look.

[fwereade, 2013-02-27 11:01:25]

LGTM.

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

https://codereview.appspot.com/7390043/diff/21002/state/api/apiclient.go#newc...
state/api/apiclient.go:227: // the watcher will die with all resources cleaned
up.
Thanks, this tweak really makes it clearer for me.

[rog, 2013-02-27 11:08:53]

*** Submitted:

state/api: implement machine watching

This watcher is simpler than some because no
information is sent with each change notification.
It shows the basic pattern though.

R=dimitern, fwereade
CC=
https://codereview.appspot.com/7390043