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

Issue 11666047: worker/machiner: Use the API instead of state (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 3 months ago by dimitern
Modified:
6 years, 3 months ago
Reviewers:
mp+176221, rog, gz
Visibility:
Public.

Description

worker/machiner: Use the API instead of state With this change, the machiner worker becomes the first one to use the API exclusively. There are still some usage of the state package, but only inside the tests. https://code.launchpad.net/~dimitern/juju-core/076-machiner-uses-api/+merge/176221 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 10

Patch Set 2 : worker/machiner: Use the API instead of state #

Unified diffs Side-by-side diffs Delta from patch set Stats (+89 lines, -71 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/machine.go View 1 2 chunks +5 lines, -5 lines 0 comments Download
M state/api/machiner/machine.go View 2 chunks +8 lines, -2 lines 0 comments Download
M worker/machiner/machiner.go View 1 3 chunks +23 lines, -29 lines 0 comments Download
M worker/machiner/machiner_test.go View 3 chunks +51 lines, -35 lines 0 comments Download

Messages

Total messages: 4
dimitern
Please take a look.
6 years, 3 months ago (2013-07-22 15:07:28 UTC) #1
rog
LGTM with some suggestions below. very nice. https://codereview.appspot.com/11666047/diff/1/cmd/jujud/machine.go File cmd/jujud/machine.go (right): https://codereview.appspot.com/11666047/diff/1/cmd/jujud/machine.go#newcode170 cmd/jujud/machine.go:170: // Only ...
6 years, 3 months ago (2013-07-22 15:19:18 UTC) #2
gz
LGTM
6 years, 3 months ago (2013-07-22 15:29:16 UTC) #3
dimitern
6 years, 3 months ago (2013-07-22 18:03:01 UTC) #4
Please take a look.

https://codereview.appspot.com/11666047/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/11666047/diff/1/cmd/jujud/machine.go#newcode170
cmd/jujud/machine.go:170: // Only the machiner currently connect to the API.
On 2013/07/22 15:19:18, rog wrote:
> s/connect/connects/

Done.

https://codereview.appspot.com/11666047/diff/1/state/api/machiner/machine.go
File state/api/machiner/machine.go (right):

https://codereview.appspot.com/11666047/diff/1/state/api/machiner/machine.go#...
state/api/machiner/machine.go:55: if result.Errors[0] != nil {
On 2013/07/22 15:19:18, rog wrote:
> alternatively we could write a function in params:
> 
> func (e *Error) Err() error {
>      if e == nil {
>           return nil
>      }
>      return e
> }
> 
> and return results.Errors[0].Err() here and elsewhere.

Excellent idea! Will do it, along with a few other tests cleanups in a follow
up.

https://codereview.appspot.com/11666047/diff/1/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/11666047/diff/1/worker/machiner/machiner.go#ne...
worker/machiner/machiner.go:47: } else if err != nil {
On 2013/07/22 15:19:18, rog wrote:
> lose the else (no need for it AFAICS)?

Did that while debugging and wasn't sure why the tests were failing for a while
- it was due to the (*params.Error)(nil) != nil. Will remove.

https://codereview.appspot.com/11666047/diff/1/worker/machiner/machiner.go#ne...
worker/machiner/machiner.go:67: logger.Errorf("%s falied to refresh: %v", mr,
err)
On 2013/07/22 15:19:18, rog wrote:
> i don't think there's a need to log the errors here - won't they be logged
when
> the notify worker quits with that error?

Removed.

https://codereview.appspot.com/11666047/diff/1/worker/machiner/machiner.go#ne...
worker/machiner/machiner.go:85: logger.Infof("%q shutting down", mr.tag)
On 2013/07/22 15:19:18, rog wrote:
> again, probably not necessary, as the runner logs when workers exit.

Removed.
Sign in to reply to this message.

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