state/apiserver: Implement Machiner.Watch

state/apiserver: Implement Machiner.Watch

Adopting Dimiter's branch, with merge conflicts on trunk resolved.

See <https://codereview.appspot.com/9937045/> for earlier discussion.

As I understand it, William would like a couple more tests, but all
other comments were addressed.

https://code.launchpad.net/~gz/juju-core/057-api-machiner-watch/+merge/170586

(do not edit description out of merge proposal)

[gz, 2013-06-20 11:16:18]

Please take a look.

[rog, 2013-06-20 13:37:26]

LGTM with a couple of trivial suggestions.

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner.go:15: resourceRegistry common.ResourceRegistry
s/resourceRegistry/resources/ ?

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:19: resourcesRegistry
fakeResourceRegistry
s/resourcesRegistry/resources/ ?

[rog, 2013-06-20 15:29:34]

one thought:

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

https://codereview.appspot.com/10439043/diff/1/state/api/params/params.go#new...
state/api/params/params.go:70: type MachinesWatchResults struct {
i think this should probably be EntityWatchResults, as anything that returns an
entity watcher will want to use the same type.

[gz, 2013-06-20 16:01:30]

Please take a look.

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

https://codereview.appspot.com/10439043/diff/1/state/api/params/params.go#new...
state/api/params/params.go:70: type MachinesWatchResults struct {
On 2013/06/20 15:29:34, rog wrote:
> i think this should probably be EntityWatchResults, as anything that returns
an
> entity watcher will want to use the same type.

Done, have left MachinesWatchResults named as currently. I'm also not crazy
about the construction of the array being done in the apiserver module having to
know about the array/etc from params to construct one of a suitable length, but
this is all old territory and let's not go there.

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner.go:15: resourceRegistry common.ResourceRegistry
On 2013/06/20 13:37:26, rog wrote:
> s/resourceRegistry/resources/ ?

Done.

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner_test.go:19: resourcesRegistry
fakeResourceRegistry
On 2013/06/20 13:37:26, rog wrote:
> s/resourcesRegistry/resources/ ?

Done.

[rog, 2013-06-20 16:13:25]

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

https://codereview.appspot.com/10439043/diff/1/state/api/params/params.go#new...
state/api/params/params.go:70: type MachinesWatchResults struct {
On 2013/06/20 16:01:30, gz wrote:
> On 2013/06/20 15:29:34, rog wrote:
> > i think this should probably be EntityWatchResults, as anything that returns
> an
> > entity watcher will want to use the same type.
> 
> Done, have left MachinesWatchResults named as currently. I'm also not crazy
> about the construction of the array being done in the apiserver module having
to
> know about the array/etc from params to construct one of a suitable length,
but
> this is all old territory and let's not go there.

I think that EntityWatchResults is probably right actually.
Any bulk call that watches a set of entities will be able to use it, and that
seems reasonable to me.

I'm not sure what you mean about the array construction. Surely we can't hide
the fact that there's a slice here?

[gz, 2013-06-20 16:30:40]

On 2013/06/20 16:13:25, rog wrote:
> 
> I think that EntityWatchResults is probably right actually.
> Any bulk call that watches a set of entities will be able to use it, and that
> seems reasonable to me.
> 
> I'm not sure what you mean about the array construction. Surely we can't hide
> the fact that there's a slice here?

I think it depends how you view that outer struct. Per the comment, I was
envisioning it as a "thing you pass to a particular api call to get results", in
which case you want one per api call, even if there happens to be overlap. Also,
you then perhaps don't want the caller having to know all the details of the
struct in order to create one and pass it in. Maybe that's the wrong way of
looking at it?

[wallyworld, 2013-06-21 01:03:09]

LGTM. I have no real view on the array question as I'm not fully across the
nuances of it.

[rog, 2013-06-21 06:31:34]

On 20 June 2013 17:30,  <martin.packman@canonical.com> wrote:
> On 2013/06/20 16:13:25, rog wrote:
>
>> I think that EntityWatchResults is probably right actually.
>> Any bulk call that watches a set of entities will be able to use it,
>
> and that
>>
>> seems reasonable to me.
>
>
>> I'm not sure what you mean about the array construction. Surely we
>
> can't hide
>>
>> the fact that there's a slice here?
>
>
> I think it depends how you view that outer struct. Per the comment, I
> was envisioning it as a "thing you pass to a particular api call to get
> results", in which case you want one per api call, even if there happens
> to be overlap. Also, you then perhaps don't want the caller having to
> know all the details of the struct in order to create one and pass it
> in. Maybe that's the wrong way of looking at it?

It's really just the return value of an API call. It needs a separate type
if the API call is returning something that no other API call returns,
but otherwise it's fine to share.

If we later need to add some other stuff to the return value for
a particular API call that shares a return type with another,
it's easy to change the return type - it's not part of the wire
contract and the return type can be changed, and a the other
stuff added, with no more compatibility
issues than adding a field to a non-shared return type.

In a way I think it's nice to share when possible as it's less lines
of code and easier to see the commonality (also easier to
write generic helper functions).

BTW the caller doesn't have to know the details of the struct
to pass it in. They do of course have to know the details of
the struct in order to deal with the results though.

[fwereade, 2013-06-21 10:21:21]

On 2013/06/21 06:31:34, rog wrote:
> It's really just the return value of an API call. It needs a separate type
> if the API call is returning something that no other API call returns,
> but otherwise it's fine to share.

+1

[fwereade, 2013-06-21 10:22:48]

Commented where I think we need better testing. Otherwise LGTM.

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machi...
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machi...
state/apiserver/machine/machiner_test.go:173: 
Can we perhaps call Next with the valid watcher's id and see what we get?

[gz, 2013-06-24 12:56:02]

Please take a look.

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machi...
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machi...
state/apiserver/machine/machiner_test.go:173: 
On 2013/06/21 10:22:48, fwereade wrote:
> Can we perhaps call Next with the valid watcher's id and see what we get?

I've had a crack at doing something with the watcher, if you can give me more of
a hint as to what we want I can improve the check.

[fwereade, 2013-06-24 13:15:56]

On 2013/06/24 12:56:02, gz wrote:
> Please take a look.
> 
>
https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machi...
> File state/apiserver/machine/machiner_test.go (right):
> 
>
https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machi...
> state/apiserver/machine/machiner_test.go:173: 
> On 2013/06/21 10:22:48, fwereade wrote:
> > Can we perhaps call Next with the valid watcher's id and see what we get?
> 
> I've had a crack at doing something with the watcher, if you can give me more
of
> a hint as to what we want I can improve the check.

LGTM, as discussed, but please read the first event off Changes() before
returning from Watch(); then do some basic verification of the watcher's state
with something like NotifyWatcherC.

[gz, 2013-06-25 10:25:11]

Please take a look.

[rog, 2013-06-25 10:47:59]

LGTM

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/ma...
File state/apiserver/machine/machiner_test.go (right):

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/ma...
state/apiserver/machine/machiner_test.go:180: err = resource.Stop()
s/=/:=

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/ma...
state/apiserver/machine/machiner_test.go:186: // Should use helpers from
state/watcher_test.go when generalised
we've got loads of places that read from a channel without a timeout. if we
generalise the code, we'll look for and change lots of them, so i don't think
this comment is really necessary.

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/ma...
state/apiserver/machine/machiner_test.go:188: case _, ok := <-channel:
this isn't actually checking that the watcher is watching the correct machine,
but i'm ok if we leave that for the client-side test.

https://codereview.appspot.com/10439043/diff/21001/state/apiserver/machine/ma...
state/apiserver/machine/machiner_test.go:191: case <-time.After(50 *
time.Millisecond):
2-5 seconds would be better. this is the worst case, and we want to be resilient
to slow machines.