Code review comment for lp://staging/~aramh/juju-core/121-state-machiner-watchers-machine-principals7
Revision history for this message
| Aram Hăvărneanu (aramh) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
9199653
demo site
(Get the code!)
Please take a look.
https:/
File cmd/jujud/
https:/
cmd/jujud/
&a.Conf.StateInfo, a.Conf.DataDir)
> i think st should probably be the first argument here, so the workers
are
> consistent in that respect.
I agree. Done.
https:/
File state/machine_
https:/
state/machine_
On 2012/11/09 09:08:08, rog wrote:
> can we please leave this in the same place, please, so i can easily
see what has
> remained the same?
Done.
https:/
state/machine_
On 2012/11/09 09:08:08, rog wrote:
> just nil?
No, DeepEquals fails with just nil.
https:/
File state/unit.go (right):
https:/
state/unit.go:409: func (u *Unit) IsAssignedTo(m *Machine) bool {
On 2012/11/09 09:08:08, rog wrote:
> is this necessary? can't we just use AssignedMachineId instead?
It was impossible to use the previous version of AssignedMachineId, but
I fixed that function instead to return usable errors and I deleted
this.
https:/
File state/watcher.go (right):
https:/
state/watcher.
> please can we put code block moves in their own CL? i don't
> disagree on principle (it is indeed better next to the
> MachineUnitsWat
Sorry, no, it was intentional. This is 100% new code, it's not
a variation of the old code. It's a variation of the current
MachineUnitsWatcher code.
https:/
state/watcher.
it may be freely refreshed
> i wonder if we should have a Clone or Copy method on Machine, Unit,
etc.
I wish we had, but niemeyer rules against it in one of the Lisbon
sprints, didn't he?
https:/
state/watcher.
updateMachine(
> i found the name "pending" slightly confusing here. how about using
"changed"
> and "newChanged" throughout (replacing "pending" and "new")? then the
> relationship between updateMachine, merge and the main loop becomes
clearer, i
> think.
I agree, but what about the MachineUnitsWat
subset of that. If we change anything here, we should change it
everywhere.
https:/
state/watcher.
On 2012/11/09 09:08:08, rog wrote:
> could we just use w.st.Unit(unit) here? then the logic below could
just use the
> normal Unit methods (e.g. IsPrincipal, AssignedMachineId).
I don't know, but see above.
https:/
state/watcher.
w.machine.doc.Id, w.machine.
On 2012/11/09 09:08:08, rog wrote:
> could we just use MachineWatcher here?
See above.
Also, I don't want to change anything in this watcher, and the machine
units watcher. The implementation will change completely in a few days
once we have containers inside state.
https:/
state/watcher.
> s/[]string(
No, clients really expect []string(nil). Perhaps we should fix teh
clients but that's outside of the scope of this CL.
https:/
File worker/
https:/
worker/
st *state.State, info *state.Info, dataDir string) *Machiner {
On 2012/11/09 09:08:08, rog wrote:
> make st the first arg.
Done.
https:/
worker/
st *state.State, info *state.Info, dataDir string, cont
container.
On 2012/11/09 09:08:08, rog wrote:
> ditto.
Done.
https:/
worker/
On 2012/11/09 09:08:08, rog wrote:
> assigned := false ?
Done.
https:/
worker/
m.stateInfo, m.tools); err != nil {
On 2012/11/09 09:08:08, rog wrote:
> i'm not sure this is quite right. if the unit's life state changes
from Alive to
> Dying, the unit will still appear to be assigned to the machine, and
we're going
> to call Deploy again, even though the unit is already in the units
map.
> i think we should check that the unit is not in the units map before
calling
> Deploy.
Done.
https:/
File worker/
https:/
worker/
On 2012/11/09 09:08:08, rog wrote:
> we need some tests that check what happens when lifecycle changes
happen here. i
> suggest adding the tests before changing the code to verify that
current
> behaviour is actually broken.
Done.
https:/