state/watcher: RelationUnitsChange -Joined

state/watcher: RelationUnitsChange -Joined

Removed the Joined field from RelationsUnitsChange
struct, used by the RelationUnitsWatcher, because
it's not used anywhere and not properly tested.

Added a note about UnitSettings.Settings field
being unreliable as well, and probably needs
removal as well.

https://code.launchpad.net/~dimitern/juju-core/086-uniter-remove-relationunit-joined/+merge/178263

(do not edit description out of merge proposal)

[dimitern, 2013-08-02 11:18:00]

Please take a look.

[fwereade, 2013-08-02 11:25:53]

LGTM modulo inelegant doc. GTG.

https://codereview.appspot.com/12344043/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12344043/diff/1/state/unit.go#newcode64
state/unit.go:64: // refreshed by the hook queue. We need to remove it in the
future.
"Settings field may always be nil and should never be depended upon."?

(hook queue is not relevant here, I think)

(and the API level is where I *really really* care about having it clearly
documented)

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

https://codereview.appspot.com/12344043/diff/1/state/watcher.go#newcode519
state/watcher.go:519: // events will be sent for that counterpart unit.
Gaah, GTG lunch, but I think this can be made a bit clearer. If nothing springs
to mind don't worry.

[rog, 2013-08-02 11:27:36]

LGTM with a doc update (and +1 to fwereade's suggestion
of making the RelationUnitsChange docs clearer)

https://codereview.appspot.com/12344043/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12344043/diff/1/state/unit.go#newcode63
state/unit.go:63: // populated and up-to-date at hook execution time, and if nil
will be
I don't think this remark makes sense in the context
of the state package, which knows nothing of hook execution.

Are you trying to say that the unit settings
in a RelationUnitsChange struct are not guaranteed
to be valid when delivered by the watcher?

If so, I suggest putting a comment to that effect on
RelationUnitsChange.

[dimitern, 2013-08-02 12:27:55]

Please take a look.

https://codereview.appspot.com/12344043/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12344043/diff/1/state/unit.go#newcode63
state/unit.go:63: // populated and up-to-date at hook execution time, and if nil
will be
On 2013/08/02 11:27:36, rog wrote:
> I don't think this remark makes sense in the context
> of the state package, which knows nothing of hook execution.
> 
> Are you trying to say that the unit settings
> in a RelationUnitsChange struct are not guaranteed
> to be valid when delivered by the watcher?
> 
> If so, I suggest putting a comment to that effect on
> RelationUnitsChange.

Sorry, what I tried to say William phrased better, I'll use his suggestion.

https://codereview.appspot.com/12344043/diff/1/state/unit.go#newcode64
state/unit.go:64: // refreshed by the hook queue. We need to remove it in the
future.
On 2013/08/02 11:25:53, fwereade wrote:
> "Settings field may always be nil and should never be depended upon."?
> 
> (hook queue is not relevant here, I think)
> 
> (and the API level is where I *really really* care about having it clearly
> documented)

Done.

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

https://codereview.appspot.com/12344043/diff/1/state/watcher.go#newcode519
state/watcher.go:519: // events will be sent for that counterpart unit.
On 2013/08/02 11:25:53, fwereade wrote:
> Gaah, GTG lunch, but I think this can be made a bit clearer. If nothing
springs
> to mind don't worry.

Done.

[rog, 2013-08-02 12:36:07]

https://codereview.appspot.com/12344043/diff/7002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/12344043/diff/7002/state/watcher.go#newcode515
state/watcher.go:515: // When a remove unit first enters scope, or its settings
are changed
much better, thanks.

how about this refinement:

// When remote units first enter scope and then when their settings
// change, the changes will be noted in the Changed field, which holds
// the unit settings for every such unit, indexed by the unit id.
//
// When remote units leave scope, their ids will be noted in the
// Departed field, and no further events will be sent for those units.

[dimitern, 2013-08-02 12:40:07]

Please take a look.

https://codereview.appspot.com/12344043/diff/7002/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/12344043/diff/7002/state/watcher.go#newcode515
state/watcher.go:515: // When a remove unit first enters scope, or its settings
are changed
On 2013/08/02 12:36:07, rog wrote:
> much better, thanks.
> 
> how about this refinement:
> 
> // When remote units first enter scope and then when their settings
> // change, the changes will be noted in the Changed field, which holds
> // the unit settings for every such unit, indexed by the unit id.
> //
> // When remote units leave scope, their ids will be noted in the
> // Departed field, and no further events will be sent for those units.

Done.

[fwereade, 2013-08-02 15:42:22]

LGTM, couple of suggestions.

https://codereview.appspot.com/12344043/diff/10003/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12344043/diff/10003/state/unit.go#newcode63
state/unit.go:63: // dependent upon. We need to remove it in the future.
s/ent/ed/

https://codereview.appspot.com/12344043/diff/10003/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/12344043/diff/10003/state/watcher.go#newcode517
state/watcher.go:517: // the unit settings for every such unit, indexed by the
unit id.
+1 to a note that the Settings are not guaranteed to be present, but will be
accurate if they are; and that the Version(?) field is always valid, and that a
change in Version always indicates a change in associated settings.

(The other one is still good, but I think a note here is even higher value, as
rog suggested.)

[dimitern, 2013-08-02 16:15:13]

On 2013/08/02 15:42:22, fwereade wrote:
> LGTM, couple of suggestions.
> 
> https://codereview.appspot.com/12344043/diff/10003/state/unit.go
> File state/unit.go (right):
> 
> https://codereview.appspot.com/12344043/diff/10003/state/unit.go#newcode63
> state/unit.go:63: // dependent upon. We need to remove it in the future.
> s/ent/ed/
> 
> https://codereview.appspot.com/12344043/diff/10003/state/watcher.go
> File state/watcher.go (right):
> 
> https://codereview.appspot.com/12344043/diff/10003/state/watcher.go#newcode517
> state/watcher.go:517: // the unit settings for every such unit, indexed by the
> unit id.
> +1 to a note that the Settings are not guaranteed to be present, but will be
> accurate if they are; and that the Version(?) field is always valid, and that
a
> change in Version always indicates a change in associated settings.
> 
> (The other one is still good, but I think a note here is even higher value, as
> rog suggested.)

Sorry, will fix there in a follow up, since this already landed.