add new methods to RelationUnit

add new methods to RelationUnit

Endpoint is required to construct JUJU_RELATION for the unit agent
Relation is also required to construct JUJU_RELATION_ID for the unit agent
ReadSettings will be used by the unit agent to read settings for departed
units, and occasionally to populate cold settings caches.

https://code.launchpad.net/~fwereade/juju-core/more-relation-unit-methods/+merge/116838

(do not edit description out of merge proposal)

[fwereade, 2012-07-26 11:32:25]

Please take a look.

[niemeyer, 2012-07-26 13:22:49]

Tastes good. Just a few twists to consider:

https://codereview.appspot.com/6440050/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode173
state/relation.go:173: // Name returns the value we expose to hooks as
JUJU_RELATION in the
This is saying that Name is the same as something else, but it doesn't actually
say what it is. It's fine to mention at the end that "This value is exposed to
hooks as the JUJU_RELATION environment variable.", but this should first be
described in terms of what it means for someone reading the documentation of
this API.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode179
state/relation.go:179: // Id returns the value we expose to hooks as
JUJU_RELATION_ID in the
Ditto.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode222
state/relation.go:222: // RemoteSettings returns a map[string]interface{}
holding the settings of
s/map[string]interface{}/map/; the types may be found in the function signature
below.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode225
state/relation.go:225: // relation, or the settings are empty; mere
non-existence of the unit is
Why is it an issue for the settings to be empty?

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode226
state/relation.go:226: // not grounds for an error, because we guarantee that
relation unit settings
s/we .* lifetime/the unit settings are guaranteed to be available for the
lifetime/; public documentation should be impersonal.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode228
state/relation.go:228: func (ru *RelationUnit) RemoteSettings(unit string)
(settings map[string]interface{}, err error) {
Okay, so here a perhaps nitpicky proposal to go a bit more in the direction we
discussed: we can move RemoteSettings to the Relation itself, and introduce a
Relation method into the RelationUnit so we can navigate back.

Also, just for clarity, on the parameter: s/unit/unitName/

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode242
state/relation.go:242: return nil, fmt.Errorf("unit is not remote")
With the mentioned refactoring, this can go away.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode244
state/relation.go:244: parts := strings.Split(unit, "/")
I think this is parseUnitName

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode252
state/relation.go:252: skey, err := topo.ServiceKey(sname)
Why do we not care if the unit exists, but care if its service exists?

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode260
state/relation.go:260: ukey := makeUnitKey(skey, useq)
Isn't ukey == unit?

[niemeyer, 2012-07-26 13:24:36]

https://codereview.appspot.com/6440050/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode228
state/relation.go:228: func (ru *RelationUnit) RemoteSettings(unit string)
(settings map[string]interface{}, err error) {
Sorry, I meant ReadSettings method on Relation.

[fwereade, 2012-07-26 14:24:28]

Please take a look.

https://codereview.appspot.com/6440050/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode173
state/relation.go:173: // Name returns the value we expose to hooks as
JUJU_RELATION in the
On 2012/07/26 13:22:49, niemeyer wrote:
> This is saying that Name is the same as something else, but it doesn't
actually
> say what it is. It's fine to mention at the end that "This value is exposed to
> hooks as the JUJU_RELATION environment variable.", but this should first be
> described in terms of what it means for someone reading the documentation of
> this API.

Done.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode179
state/relation.go:179: // Id returns the value we expose to hooks as
JUJU_RELATION_ID in the
On 2012/07/26 13:22:49, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode222
state/relation.go:222: // RemoteSettings returns a map[string]interface{}
holding the settings of
On 2012/07/26 13:22:49, niemeyer wrote:
> s/map[string]interface{}/map/; the types may be found in the function
signature
> below.

Done.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode225
state/relation.go:225: // relation, or the settings are empty; mere
non-existence of the unit is
On 2012/07/26 13:22:49, niemeyer wrote:
> Why is it an issue for the settings to be empty?

As discussed, we should always have a "private-address" key to allow other units
to talk to it.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode226
state/relation.go:226: // not grounds for an error, because we guarantee that
relation unit settings
On 2012/07/26 13:22:49, niemeyer wrote:
> s/we .* lifetime/the unit settings are guaranteed to be available for the
> lifetime/; public documentation should be impersonal.

Done.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode228
state/relation.go:228: func (ru *RelationUnit) RemoteSettings(unit string)
(settings map[string]interface{}, err error) {
On 2012/07/26 13:24:36, niemeyer wrote:
> Sorry, I meant ReadSettings method on Relation.

Whoops, it turns out we can't do this: for posterity, we need the RelationUnit's
scope to figure out the right path for the settings, because for
container-scoped relations we need to know the container to figure out the
settings path.

ReadSettings is definitely a better name, though.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode242
state/relation.go:242: return nil, fmt.Errorf("unit is not remote")
On 2012/07/26 13:22:49, niemeyer wrote:
> With the mentioned refactoring, this can go away.

Done.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode244
state/relation.go:244: parts := strings.Split(unit, "/")
On 2012/07/26 13:22:49, niemeyer wrote:
> I think this is parseUnitName

Ha, I *thought* there should be *something* like that :/. Cheers. Done.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode252
state/relation.go:252: skey, err := topo.ServiceKey(sname)
On 2012/07/26 13:22:49, niemeyer wrote:
> Why do we not care if the unit exists, but care if its service exists?

We need the service key to build a unit key.

https://codereview.appspot.com/6440050/diff/1/state/relation.go#newcode260
state/relation.go:260: ukey := makeUnitKey(skey, useq)
On 2012/07/26 13:22:49, niemeyer wrote:
> Isn't ukey == unit?

Key, not name.

[niemeyer, 2012-07-26 15:15:02]

Last round:

https://codereview.appspot.com/6440050/diff/5001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6440050/diff/5001/state/relation.go#newcode157
state/relation.go:157: role:     ep.RelationRole,
Sorry for not realizing this earlier, but what about storing ep itself, and
making it available instead of Name below?

The documentation and meaning then become straightforward:

// Endpoint returns the relation endpoint associated with the unit
// participation in this relation.

https://codereview.appspot.com/6440050/diff/5001/state/relation.go#newcode181
state/relation.go:181: // Id returns the relation's id according to the unit.
This value is
// RelationId returns a descriptor that uniquely identifies the relation
// from the perspective of the unit. Different RelationUnit values may
// see the same RelationId result. This value is exposed to hooks as
// the JUJU_RELATION_ID environment variable.

https://codereview.appspot.com/6440050/diff/5001/state/relation.go#newcode260
state/relation.go:260: if address, found := node.Get("private-address"); found {
We discussed this live.

[fwereade, 2012-07-26 16:02:45]

Please take a look.

https://codereview.appspot.com/6440050/diff/5001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6440050/diff/5001/state/relation.go#newcode157
state/relation.go:157: role:     ep.RelationRole,
On 2012/07/26 15:15:02, niemeyer wrote:
> Sorry for not realizing this earlier, but what about storing ep itself, and
> making it available instead of Name below?
> 
> The documentation and meaning then become straightforward:
> 
> // Endpoint returns the relation endpoint associated with the unit
> // participation in this relation.

Nice; as discussed, also adding Relation and dropping Id, because it feels a bit
off.

https://codereview.appspot.com/6440050/diff/5001/state/relation.go#newcode181
state/relation.go:181: // Id returns the relation's id according to the unit.
This value is
On 2012/07/26 15:15:02, niemeyer wrote:
> // RelationId returns a descriptor that uniquely identifies the relation
> // from the perspective of the unit. Different RelationUnit values may
> // see the same RelationId result. This value is exposed to hooks as
> // the JUJU_RELATION_ID environment variable.

Dropping this, as discussed; we can build this later by using Relation and
Endpoint.

https://codereview.appspot.com/6440050/diff/5001/state/relation.go#newcode260
state/relation.go:260: if address, found := node.Get("private-address"); found {
On 2012/07/26 15:15:02, niemeyer wrote:
> We discussed this live.

Done.

[niemeyer, 2012-07-26 18:02:39]

LGTM with a few trivials. Thanks!

https://codereview.appspot.com/6440050/diff/8001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6440050/diff/8001/state/relation.go#newcode176
state/relation.go:176: // Endpoint returns the relation endpoint associated with
the unit.
The previously recommended comment sounds slightly more correct:

// Endpoint returns the relation endpoint associated with the unit
// participation in this relation.

https://codereview.appspot.com/6440050/diff/8001/state/relation.go#newcode250
state/relation.go:250: if stat, err := ru.st.zk.Exists(path); err != nil {
A second roundtrip is a pity. I suggest adding a trivial "existed" flag onto
*ConfigNode that we explore here in a semi-hackish way for the moment. mstate
will hopefully get us over it, and if not we add a better mechanism later.

https://codereview.appspot.com/6440050/diff/8001/state/relation.go#newcode253
state/relation.go:253: return nil, fmt.Errorf("unit settings do not exist")
"settings for unit %q not found in relation %q"

[fwereade, 2012-07-26 19:02:04]

*** Submitted:

add new methods to RelationUnit

Endpoint is required to construct JUJU_RELATION for the unit agent
Relation is also required to construct JUJU_RELATION_ID for the unit agent
ReadSettings will be used by the unit agent to read settings for departed
units, and occasionally to populate cold settings caches.

R=niemeyer
CC=
https://codereview.appspot.com/6440050

https://codereview.appspot.com/6440050/diff/8001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6440050/diff/8001/state/relation.go#newcode176
state/relation.go:176: // Endpoint returns the relation endpoint associated with
the unit.
On 2012/07/26 18:02:39, niemeyer wrote:
> The previously recommended comment sounds slightly more correct:
> 
> // Endpoint returns the relation endpoint associated with the unit
> // participation in this relation.

...the relation endpoint that defines the unit's participation in the relation.

https://codereview.appspot.com/6440050/diff/8001/state/relation.go#newcode250
state/relation.go:250: if stat, err := ru.st.zk.Exists(path); err != nil {
On 2012/07/26 18:02:39, niemeyer wrote:
> A second roundtrip is a pity. I suggest adding a trivial "existed" flag onto
> *ConfigNode that we explore here in a semi-hackish way for the moment. mstate
> will hopefully get us over it, and if not we add a better mechanism later.

I don't expect to hit this method very often -- only when (1) we want settings
for a unit that is not a relation member (and even then at most once per hook)
and (2) we've just started up a unit agent and have not yet been fed cached
settings by the relation's HookQueue (at most once per process). Given the
expected rarity of those situations, I think it's better to hit zookeeper more
and dirty up ConfigNode less.

https://codereview.appspot.com/6440050/diff/8001/state/relation.go#newcode253
state/relation.go:253: return nil, fmt.Errorf("unit settings do not exist")
On 2012/07/26 18:02:39, niemeyer wrote:
> "settings for unit %q not found in relation %q"

Entities already named in errorContextf.