Drop ServiceRelation; add Relation

Drop ServiceRelation; add Relation

As discussed on IRC, ServiceRelation is "difficult" and liable to lead to
confusion; and the code has been hinting that a Relation really is just a
list of endpoints at heart. You'd think this would be obvious to anyone, in
hindsight :).

The proposed ordering of endpoints within a relation[0] has been dropped, in
favour of a pair of Relation methods that expose the same information but
more explicitly (Endpoint/RelatedEndpoint).

[0] Early discussions suggested that Service.Relations should return a slice
of slices of endpoints, with the latter ordered such that the first endpoint
corresponded to the service from which the relations were acquired.

https://code.launchpad.net/~fwereade/juju-core/relation-type/+merge/110870

(do not edit description out of merge proposal)

[fwereade, 2012-06-18 17:31:54]

Please take a look.

[niemeyer, 2012-06-18 19:13:00]

We've discussed ideas around this online today.

[fwereade, 2012-06-18 22:33:46]

Please take a look.

[fwereade, 2012-06-20 11:11:42]

Please take a look.

[niemeyer, 2012-06-20 15:15:05]

Almost there!

https://codereview.appspot.com/6312048/diff/4008/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode20
state/relation.go:20: // relate to.
// This should remain an internal method because the relation
// model does not guarantee that for every role there will
// necessarily exist a single counterpart role that is sensible
// for basing algorithms upon.

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode89
state/relation.go:89: // running a relation hook on a unit of the supplied
service.
This isn't documenting what this function returns. It's documenting that what it
returns is the same as something else.

By the way, ideally new functionality unrelated to the review points made
shouldn't be introduced mid-review, otherwise we risk running around in circles
a bit.

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode90
state/relation.go:90: func (r *Relation) Id(serviceName string) (string, error)
{
Hmmm.. what about dropping the serviceName argument, and returning the numeric
id only?

It's a bit strange to have the serviceName parameter in an Id method, and it's
also a bit strange to have a different relation id to each participant server
from the perspective of this API.

We can preserve the properties of JUJU_RELATION_ID within the unit by merely
joining the numeric id to the local relation name.

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode103
state/relation.go:103: // Endpoint returns the endpoint of the relation attached
to the named service.
s/attached to/for/

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode115
state/relation.go:115: // named service will relate. If the service is not part
of the relation, an
s/relate/establish relations with/, just for slightly more clarity.

Also, s/of the relation/of the r relation/, to disambiguate.

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode124
state/relation.go:124: for _, ep := range r.endpoints {
Nice logic here.

https://codereview.appspot.com/6312048/diff/4008/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6312048/diff/4008/state/service.go#newcode221
state/service.go:221: var err error
This will conflict with Frank's change. Watch out as this is broken. err must be
a named result for errorContextf to work.

https://codereview.appspot.com/6312048/diff/4008/state/service.go#newcode231
state/service.go:231: relations := []*Relation{}
s/relations/rs/, to follow along everything else below.

https://codereview.appspot.com/6312048/diff/4008/state/service.go#newcode235
state/service.go:235: for skey, rs := range tr.Services {
rs is not a good name here for something that isn't "relations" here due to
context.

[niemeyer, 2012-06-20 15:21:44]

Actually, LGTM with that in.

[fwereade, 2012-06-20 16:25:16]

*** Submitted:

Drop ServiceRelation; add Relation

As discussed on IRC, ServiceRelation is "difficult" and liable to lead to
confusion; and the code has been hinting that a Relation really is just a
list of endpoints at heart. You'd think this would be obvious to anyone, in
hindsight :).

The proposed ordering of endpoints within a relation[0] has been dropped, in
favour of a pair of Relation methods that expose the same information but
more explicitly (Endpoint/RelatedEndpoint).

[0] Early discussions suggested that Service.Relations should return a slice
of slices of endpoints, with the latter ordered such that the first endpoint
corresponded to the service from which the relations were acquired.

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

https://codereview.appspot.com/6312048/diff/4008/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode20
state/relation.go:20: // relate to.
On 2012/06/20 15:15:05, niemeyer wrote:
> // This should remain an internal method because the relation
> // model does not guarantee that for every role there will
> // necessarily exist a single counterpart role that is sensible
> // for basing algorithms upon. 

Done.

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode89
state/relation.go:89: // running a relation hook on a unit of the supplied
service.
On 2012/06/20 15:15:05, niemeyer wrote:
> This isn't documenting what this function returns. It's documenting that what
it
> returns is the same as something else.
> 
> By the way, ideally new functionality unrelated to the review points made
> shouldn't be introduced mid-review, otherwise we risk running around in
circles
> a bit.

Done.

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode90
state/relation.go:90: func (r *Relation) Id(serviceName string) (string, error)
{
On 2012/06/20 15:15:05, niemeyer wrote:
> Hmmm.. what about dropping the serviceName argument, and returning the numeric
> id only?
> 
> It's a bit strange to have the serviceName parameter in an Id method, and it's
> also a bit strange to have a different relation id to each participant server
> from the perspective of this API.
> 
> We can preserve the properties of JUJU_RELATION_ID within the unit by merely
> joining the numeric id to the local relation name.

Done.

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode103
state/relation.go:103: // Endpoint returns the endpoint of the relation attached
to the named service.
On 2012/06/20 15:15:05, niemeyer wrote:
> s/attached to/for/

Done.

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode115
state/relation.go:115: // named service will relate. If the service is not part
of the relation, an
On 2012/06/20 15:15:05, niemeyer wrote:
> s/relate/establish relations with/, just for slightly more clarity.
> 
> Also, s/of the relation/of the r relation/, to disambiguate.

Done.

https://codereview.appspot.com/6312048/diff/4008/state/relation.go#newcode124
state/relation.go:124: for _, ep := range r.endpoints {
On 2012/06/20 15:15:05, niemeyer wrote:
> Nice logic here.
Cheers :).

https://codereview.appspot.com/6312048/diff/4008/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6312048/diff/4008/state/service.go#newcode221
state/service.go:221: var err error
On 2012/06/20 15:15:05, niemeyer wrote:
> This will conflict with Frank's change. Watch out as this is broken. err must
be
> a named result for errorContextf to work.

Yeah, that makes perfect sense now I think about it :/. Thanks.

https://codereview.appspot.com/6312048/diff/4008/state/service.go#newcode231
state/service.go:231: relations := []*Relation{}
On 2012/06/20 15:15:05, niemeyer wrote:
> s/relations/rs/, to follow along everything else below.

On further thought... I think there are enough error paths to make
errorfContextf worthwhile, which implies named parameters, which then rather
demands readably-named ones.

https://codereview.appspot.com/6312048/diff/4008/state/service.go#newcode235
state/service.go:235: for skey, rs := range tr.Services {
On 2012/06/20 15:15:05, niemeyer wrote:
> rs is not a good name here for something that isn't "relations" here due to
> context.

I think I've just realised what topoRelation.Services is... it's
topoRelation.Endpoints. (Or, at least, the remaining information necessary to
reconstruct the endpoints.)

I'm calling it "tep" for now, and I'm going to propose a tiny followup that
fixes topoRelationService (and related bits') naming.