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

Issue 6851110: state: consolidate lifecycles watchers

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 4 months ago by fwereade
Modified:
11 years, 4 months ago
Reviewers:
mp+136206
Visibility:
Public.

Description

state: consolidate lifecycles watchers State.WatchServices and state.WatchMachines now share a common implementation. https://code.launchpad.net/~fwereade/juju-core/consolidate-lifecycles-watchers/+merge/136206 Requires: https://code.launchpad.net/~fwereade/juju-core/machine-string-ids/+merge/135423 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 17

Patch Set 2 : state: consolidate lifecycles watchers #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -175 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M state/state_test.go View 1 1 chunk +3 lines, -7 lines 0 comments Download
M state/watcher.go View 1 4 chunks +80 lines, -167 lines 0 comments Download
M worker/firewaller/firewaller.go View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 5
fwereade
Please take a look.
11 years, 4 months ago (2012-11-26 15:55:23 UTC) #1
niemeyer
LGTM. Just high-level suggestions: https://codereview.appspot.com/6851110/diff/1/state/watcher.go File state/watcher.go (right): https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode81 state/watcher.go:81: // LifecyclesWatcher notifies about lifecycle ...
11 years, 4 months ago (2012-11-28 13:34:44 UTC) #2
rog
LGTM https://codereview.appspot.com/6851110/diff/1/state/watcher.go File state/watcher.go (right): https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode81 state/watcher.go:81: // LifecyclesWatcher notifies about lifecycle changes for all ...
11 years, 4 months ago (2012-11-28 14:43:28 UTC) #3
niemeyer
https://codereview.appspot.com/6851110/diff/1/state/watcher.go File state/watcher.go (right): https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode127 state/watcher.go:127: // Changes returns the event channel for the LifecyclesWatcher. ...
11 years, 4 months ago (2012-11-28 15:26:35 UTC) #4
fwereade
11 years, 4 months ago (2012-11-28 16:15:43 UTC) #5
*** Submitted:

state: consolidate lifecycles watchers

State.WatchServices and state.WatchMachines now share a common implementation.

R=niemeyer, rog
CC=
https://codereview.appspot.com/6851110

https://codereview.appspot.com/6851110/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode81
state/watcher.go:81: // LifecyclesWatcher notifies about lifecycle changes for
all entities of
On 2012/11/28 13:34:44, niemeyer wrote:
> s/cycles/cycle/ sounds a bit better IMO (monitors the lifecycle of entities,
> rather than the lifecycles of entities).

Done.

https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode82
state/watcher.go:82: // a given kind. The first event emitted will contain the
ids of each such
On 2012/11/28 13:34:44, niemeyer wrote:
> s/each such non-Dead entity/all non-Dead entities/

Done.

https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode83
state/watcher.go:83: // non-Dead entity; subsequent events are emitted whenever
one such entity
On 2012/11/28 13:34:44, niemeyer wrote:
> s/one such entity is/one or more entities are/
> 
> It might be just me, but I feel that generally "such" can be replaced by
> something more flowing and better sounding.

Done.

https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode89
state/watcher.go:89: life map[string]Life
On 2012/11/28 14:43:28, rog wrote:
> a comment saying what the key is might be nice.

Done.

https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode94
state/watcher.go:94: // machines in the environment.
On 2012/11/28 13:34:44, niemeyer wrote:
> It doesn't report changes *to machines*.

Done.

https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode100
state/watcher.go:100: // services in the environment.
On 2012/11/28 13:34:44, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/6851110/diff/1/state/watcher.go#newcode164
state/watcher.go:164: if known && gone {
On 2012/11/28 14:43:28, rog wrote:
> switch?

Done.
Sign in to reply to this message.

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