Code review comment for lp:~themue/juju-core/go-worker-firewaller-machines

William Reade (fwereade) wrote :

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode16
worker/firewaller/firewaller.go:16: environWatcher
*state.ConfigWatcher
Shouldn't be a field, only used inside loop().

Also, I think I mentioned this somewhere before... why do the firewaller
and the provisioner each hold independent instances of what should be
the same Environ? Given that we can (AIUI) change environ config, and
use the environ itself, safely, at any time in any goroutine, I don't
see the benefit of duplicating the
get-me-an-environ-and-keep-it-up-to-date logic.

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode17
worker/firewaller/firewaller.go:17: machinesWatcher
*state.MachinesWatcher
Shouldn't be a field, only used inside loop().

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode42
worker/firewaller/firewaller.go:42: // Get environment first.
On 2012/07/16 16:40:46, rog wrote:
> defer watcher.Stop(fw.machineUnitsChanges, &fw.tomb)

defer close(fw.machineUnitsChanges)
defer watcher.Stop(fw.machinesWatcher, &fw.tomb)

(although as stated above I don't think machinesWatcher needs to be a
field)

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode53
worker/firewaller/firewaller.go:53: }
fw.tomb.Kill(watcher.MustErr(fw.environWatcher))

(except it shouln't be a field, and IMO shouldn't even exist here)

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode68
worker/firewaller/firewaller.go:68: }
watcher.MustErr, as above.

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode75
worker/firewaller/firewaller.go:75: // case of not managed machine?
Feels like a panic situation to me.

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode86
worker/firewaller/firewaller.go:86: fw.machines[addedMachine.Id()] = m
go machine.loop()

In fact, plausibly, m := newMachine(addedMachine)

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode129
worker/firewaller/firewaller.go:129: if !ok {
fw.tomb.Kill(watcher.MustErr(m.watcher))?

https://codereview.appspot.com/6374069/

« Back to merge proposal