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

Issue 10939043: state/api/watcher: Refactor API watchers

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 9 months ago by jameinel
Modified:
10 years, 8 months ago
Reviewers:
dimitern, mp+173035, fwereade
Visibility:
Public.

Description

state/api/watcher: Refactor API watchers This moves the state/api/watcher.go file into state/api/watcher/watcher.go. The main benefit of that is to avoid circular imports as we start having APIs themselves returning Watcher objects. In doing that move I found a few things: 1) We needed api.ErrCode for a few things, so it got moved to params.ErrCode, which caused some small duplication with the existing params.Errors. However, they were pratically identical (apierrors.go had a GoString that params.Errors didn't have.) 2) Most callers that were calling api.ErrCode already had imported params anyway. 3) AllWatcher is very entwined with its Client, so I didn't move that specific part of the code. (Client can return an AllWatcher, and AllWatcher points to its Client.) 4) I changed the Watcher code so it needs a Caller rather than an api.State. The only thing any of the code did was call .Call anyway, which means we have a nice opportunity that we could mock out actual calls to the remote State api if we want some tighter testing of the client-side code. So I like switching to a simple interface. I'm not sure about that being a "Caller" interface because that seems a bit generic. 5) api.params.NotifyWatcher implements the same interface as state.NotifyWatcher. So we can be reasonably confident that code that was using an existing watcher will be compatible. Yay. https://code.launchpad.net/~jameinel/juju-core/api-watchers/+merge/173035 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 14

Patch Set 2 : state/api/watcher: Refactor API watchers #

Patch Set 3 : state/api/watcher: Refactor API watchers #

Patch Set 4 : state/api/watcher: Refactor API watchers #

Total comments: 5

Patch Set 5 : state/api/watcher: Refactor API watchers #

Total comments: 16

Patch Set 6 : state/api/watcher: Refactor API watchers #

Unified diffs Side-by-side diffs Delta from patch set Stats (+288 lines, -284 lines) Patch
A [revision details] View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/agent.go View 1 chunk +1 line, -1 line 0 comments Download
M environs/agent/agent.go View 2 chunks +2 lines, -1 line 0 comments Download
A state/api/allwatcher.go View 1 chunk +29 lines, -0 lines 0 comments Download
M state/api/apiclient.go View 1 2 3 4 2 chunks +4 lines, -3 lines 0 comments Download
M state/api/machineagent/state_test.go View 3 chunks +3 lines, -3 lines 0 comments Download
M state/api/machiner/machiner_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M state/api/params/apierror.go View 1 2 3 4 5 3 chunks +16 lines, -7 lines 0 comments Download
M state/api/params/params.go View 2 chunks +8 lines, -22 lines 0 comments Download
M state/api/upgrader/upgrader_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M state/api/watcher/watcher.go View 1 2 3 4 5 8 chunks +26 lines, -120 lines 0 comments Download
A state/api/watcher/watcher_test.go View 1 2 3 4 5 1 chunk +123 lines, -0 lines 0 comments Download
M state/apiserver/client/perm_test.go View 1 8 chunks +8 lines, -7 lines 0 comments Download
M state/apiserver/common/errors.go View 3 chunks +15 lines, -16 lines 0 comments Download
M state/apiserver/common/password_test.go View 1 2 chunks +1 line, -2 lines 0 comments Download
M state/apiserver/errors_test.go View 1 2 chunks +15 lines, -15 lines 0 comments Download
M state/apiserver/login_test.go View 1 3 chunks +4 lines, -3 lines 0 comments Download
M state/apiserver/machine/agent_test.go View 1 3 chunks +3 lines, -4 lines 0 comments Download
M state/apiserver/machine/machiner.go View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M state/apiserver/machine/machiner_test.go View 1 2 3 4 5 6 chunks +16 lines, -25 lines 0 comments Download
M state/apiserver/root.go View 1 2 1 chunk +0 lines, -19 lines 0 comments Download
M state/apiserver/upgrader/upgrader_test.go View 1 2 3 chunks +3 lines, -3 lines 0 comments Download
M state/apiserver/watcher.go View 1 2 1 chunk +0 lines, -29 lines 0 comments Download

Messages

Total messages: 9
jameinel
Please take a look.
10 years, 9 months ago (2013-07-04 14:41:43 UTC) #1
fwereade
Looking very nice, but https://codereview.appspot.com/10939043/diff/1/state/api/watcher/watcher.go#newcode170 gives me pause. https://codereview.appspot.com/10939043/diff/1/state/api/allwatcher.go File state/api/allwatcher.go (right): https://codereview.appspot.com/10939043/diff/1/state/api/allwatcher.go#newcode29 state/api/allwatcher.go:29: } ...
10 years, 9 months ago (2013-07-04 16:13:37 UTC) #2
jameinel
Please take a look. https://codereview.appspot.com/10939043/diff/1/state/api/apiclient.go File state/api/apiclient.go (right): https://codereview.appspot.com/10939043/diff/1/state/api/apiclient.go#newcode14 state/api/apiclient.go:14: apiparams "launchpad.net/juju-core/state/api/params" On 2013/07/04 16:13:37, ...
10 years, 9 months ago (2013-07-04 20:14:47 UTC) #3
jameinel
Please take a look.
10 years, 9 months ago (2013-07-05 10:54:20 UTC) #4
jameinel
Please take a look.
10 years, 9 months ago (2013-07-05 11:10:41 UTC) #5
fwereade
LGTM https://codereview.appspot.com/10939043/diff/8002/state/api/apiclient.go File state/api/apiclient.go (left): https://codereview.appspot.com/10939043/diff/8002/state/api/apiclient.go#oldcode150 state/api/apiclient.go:150: err := s.client.Call(objType, id, request, params, response) I'd ...
10 years, 8 months ago (2013-07-08 09:15:23 UTC) #6
jameinel
Please take a look. https://codereview.appspot.com/10939043/diff/8002/state/api/apiclient.go File state/api/apiclient.go (left): https://codereview.appspot.com/10939043/diff/8002/state/api/apiclient.go#oldcode150 state/api/apiclient.go:150: err := s.client.Call(objType, id, request, ...
10 years, 8 months ago (2013-07-08 09:41:45 UTC) #7
dimitern
LGTM modulo minor suggestions. Not sure why EnvironConfigWatcher is gone though - don't we still ...
10 years, 8 months ago (2013-07-08 10:07:10 UTC) #8
jameinel
10 years, 8 months ago (2013-07-08 10:45:13 UTC) #9
Please take a look.

https://codereview.appspot.com/10939043/diff/20001/state/api/allwatcher.go
File state/api/allwatcher.go (right):

https://codereview.appspot.com/10939043/diff/20001/state/api/allwatcher.go#ne...
state/api/allwatcher.go:6: import (
On 2013/07/08 10:07:10, dimitern wrote:
> i'd put this on a single line since it's the only import, but as you wish.

I tend to go with consistent imports with other packages.
The big win here is that when you need to include 1 more package, it ends up
being a 1 line diff to review.

Rather than a 4 line diff where all the extra changes to import lines are done.
Which touches lines that aren't logically modified (you still import the same
package, but now it has to be on a separate line, with a trailing comma, etc.)

https://codereview.appspot.com/10939043/diff/20001/state/api/params/apierror.go
File state/api/params/apierror.go (left):

https://codereview.appspot.com/10939043/diff/20001/state/api/params/apierror....
state/api/params/apierror.go:8: // Error is the type of error returned by any
call
On 2013/07/08 10:07:10, dimitern wrote:
> why is this doc comment gone?

It was accidental because of a fair amount of collapsing of code. I pulled the
Error structure out of params.go and copied it into here, and then merged the
implementations. I just deleted something I shouldn't. 

Restored.

https://codereview.appspot.com/10939043/diff/20001/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (right):

https://codereview.appspot.com/10939043/diff/20001/state/api/watcher/watcher....
state/api/watcher/watcher.go:64: log.Errorf("state/api: error trying to stop
watcher %p: %v", w, err)
On 2013/07/08 10:07:10, dimitern wrote:
> why %p instead of %v?

Watcher has about 10 fields, none of which give you a concrete handle on which
watcher it is. I added it for debug tracing, I can remove it.

https://codereview.appspot.com/10939043/diff/20001/state/api/watcher/watcher_...
File state/api/watcher/watcher_test.go (right):

https://codereview.appspot.com/10939043/diff/20001/state/api/watcher/watcher_...
state/api/watcher/watcher_test.go:27: const shortTime = 50 * time.Millisecond
On 2013/07/08 10:07:10, dimitern wrote:
> doc comment? what's this used for?

checking that something is properly blocked when you expect it to be. (Next()
shouldn't return when there aren't any Changes, for example.)

Added a comment.

https://codereview.appspot.com/10939043/diff/20001/state/api/watcher/watcher_...
state/api/watcher/watcher_test.go:92: // We expect the Call() to "Next" to
block, so run it in a goroutine.
On 2013/07/08 10:07:10, dimitern wrote:
> put a new line before the comment please

Done.

https://codereview.appspot.com/10939043/diff/20001/state/api/watcher/watcher_...
state/api/watcher/watcher_test.go:98: select {
On 2013/07/08 10:07:10, dimitern wrote:
> and here before the select

Done.

https://codereview.appspot.com/10939043/diff/20001/state/apiserver/machine/ma...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/10939043/diff/20001/state/apiserver/machine/ma...
state/apiserver/machine/machiner.go:66: // Consume the initial event
On 2013/07/08 10:07:10, dimitern wrote:
> why?

I clarified the comment.

// Consume the initial event. Technically, API
// calls to Watch 'transmit' the initial event
// in the Watch response. But NotifyWatchers
// have no state to transmit.

It makes Machiner.Watch act more like LifeCycleWatcher.Watch. Where you know
there is an initial state event, it gets sent back in the Response for starting
the watcher, and the client-side of the watcher returns it in the first call to
Changes.
Sign in to reply to this message.

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