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