Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1086)

Issue 10439043: state/apiserver: Implement Machiner.Watch

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 9 months ago by gz
Modified:
10 years, 9 months ago
Reviewers:
wallyworld, mp+170586, fwereade, rog
Visibility:
Public.

Description

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)

Patch Set 1 #

Total comments: 7

Patch Set 2 : state/apiserver: Implement Machiner.Watch #

Total comments: 2

Patch Set 3 : state/apiserver: Implement Machiner.Watch #

Patch Set 4 : state/apiserver: Implement Machiner.Watch #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+126 lines, -26 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M state/api/params/params.go View 1 1 chunk +11 lines, -0 lines 0 comments Download
M state/apiserver/client.go View 1 chunk +1 line, -1 line 0 comments Download
M state/apiserver/common/interfaces.go View 1 chunk +17 lines, -0 lines 0 comments Download
M state/apiserver/machine/machiner.go View 1 2 chunks +28 lines, -8 lines 0 comments Download
M state/apiserver/machine/machiner_test.go View 1 2 3 4 chunks +56 lines, -3 lines 4 comments Download
M state/apiserver/resource.go View 4 chunks +6 lines, -10 lines 0 comments Download
M state/apiserver/root.go View 1 chunk +5 lines, -4 lines 0 comments Download

Messages

Total messages: 14
gz
Please take a look.
10 years, 9 months ago (2013-06-20 11:16:18 UTC) #1
rog
LGTM with a couple of trivial suggestions. https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machiner.go File state/apiserver/machine/machiner.go (right): https://codereview.appspot.com/10439043/diff/1/state/apiserver/machine/machiner.go#newcode15 state/apiserver/machine/machiner.go:15: resourceRegistry common.ResourceRegistry ...
10 years, 9 months ago (2013-06-20 13:37:26 UTC) #2
rog
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#newcode70 state/api/params/params.go:70: type MachinesWatchResults struct { i think this ...
10 years, 9 months ago (2013-06-20 15:29:34 UTC) #3
gz
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#newcode70 state/api/params/params.go:70: type MachinesWatchResults struct { On ...
10 years, 9 months ago (2013-06-20 16:01:30 UTC) #4
rog
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#newcode70 state/api/params/params.go:70: type MachinesWatchResults struct { On 2013/06/20 16:01:30, gz wrote: ...
10 years, 9 months ago (2013-06-20 16:13:25 UTC) #5
gz
On 2013/06/20 16:13:25, rog wrote: > > I think that EntityWatchResults is probably right actually. ...
10 years, 9 months ago (2013-06-20 16:30:40 UTC) #6
wallyworld
LGTM. I have no real view on the array question as I'm not fully across ...
10 years, 9 months ago (2013-06-21 01:03:09 UTC) #7
rog
On 20 June 2013 17:30, <martin.packman@canonical.com> wrote: > On 2013/06/20 16:13:25, rog wrote: > >> ...
10 years, 9 months ago (2013-06-21 06:31:34 UTC) #8
fwereade
On 2013/06/21 06:31:34, rog wrote: > It's really just the return value of an API ...
10 years, 9 months ago (2013-06-21 10:21:21 UTC) #9
fwereade
Commented where I think we need better testing. Otherwise LGTM. https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go File state/apiserver/machine/machiner_test.go (right): https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go#newcode173 ...
10 years, 9 months ago (2013-06-21 10:22:48 UTC) #10
gz
Please take a look. https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go File state/apiserver/machine/machiner_test.go (right): https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go#newcode173 state/apiserver/machine/machiner_test.go:173: On 2013/06/21 10:22:48, fwereade wrote: ...
10 years, 9 months ago (2013-06-24 12:56:02 UTC) #11
fwereade
On 2013/06/24 12:56:02, gz wrote: > Please take a look. > > https://codereview.appspot.com/10439043/diff/10/state/apiserver/machine/machiner_test.go > File ...
10 years, 9 months ago (2013-06-24 13:15:56 UTC) #12
gz
Please take a look.
10 years, 9 months ago (2013-06-25 10:25:11 UTC) #13
rog
10 years, 9 months ago (2013-06-25 10:47:59 UTC) #14
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b