state: Added two methods to add relations to State.

state: Added two methods to add relations to State.

AddClientServerReleation() gets a client and a server endpoint,
AddPeerRelation() a peer endpoint. They return the relation and
both service relations for client/server or the one for peer.
The seperation into two methods with well defined arguments make
it easier to ensure a valid relation creation.

https://code.launchpad.net/~themue/juju/go-state-add-relation/+merge/106652

Requires: https://code.launchpad.net/~themue/juju/go-state-topology-relation-endpoints/+merge/105156

(do not edit description out of merge proposal)

[TheMue, 2012-05-21 16:21:47]

Please take a look.

[niemeyer, 2012-05-21 20:07:47]

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

https://codereview.appspot.com/6223055/diff/1/state/relation.go#newcode60
state/relation.go:60: func (r *Relation) Key() string {
The internal key is internal. :-)

https://codereview.appspot.com/6223055/diff/1/state/relation.go#newcode73
state/relation.go:73: // Key returns the internal key of a relation.
Ditto.

https://codereview.appspot.com/6223055/diff/1/state/relation.go#newcode78
state/relation.go:78: // ServiceKey returns the service key of a relation.
Ditto.

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

https://codereview.appspot.com/6223055/diff/1/state/state.go#newcode239
state/state.go:239: if clientEp.RelationRole != RoleClient {
As a general and vague comment, visually these functions do not look nice. Maybe
there's no way to avoid the complexity, but please look for opportunities to
separate logical blocks visually, and perhaps using functions is adequate.

I'll hold-off on reviewing this until we sort out the pre-req, since there are
necessary changes there that will certainly affect this.

[TheMue, 2012-05-29 15:55:13]

Please take a look.

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

https://codereview.appspot.com/6223055/diff/1/state/relation.go#newcode60
state/relation.go:60: func (r *Relation) Key() string {
On 2012/05/21 20:07:48, niemeyer wrote:
> The internal key is internal. :-)

Done.

https://codereview.appspot.com/6223055/diff/1/state/relation.go#newcode73
state/relation.go:73: // Key returns the internal key of a relation.
On 2012/05/21 20:07:48, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/6223055/diff/1/state/relation.go#newcode78
state/relation.go:78: // ServiceKey returns the service key of a relation.
On 2012/05/21 20:07:48, niemeyer wrote:
> Ditto.

Done.

[TheMue, 2012-05-30 08:20:25]

Please take a look.

[niemeyer, 2012-05-30 22:35:42]

https://codereview.appspot.com/6223055/diff/9001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode50
state/relation.go:50: // Relation represents a connection between one or more
services.
// Relation represents a link between services, or within a
// service (in the case of peer relations).

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode56
state/relation.go:56: // ServiceRelation represents a relation between one or
more services.
// ServiceRelation represents an established relation from
// the viewpoint of a participant service.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode59
state/relation.go:59: key        string
s/key/relationKey/; given the type name, this would be ambiguous.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode63
state/relation.go:63: name       string
relationScope, relationRole, relationName, for the same reason.

Also on the methods below please (RelationRole, RelationName, etc).

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode66
state/relation.go:66: // Scope returns the scope of a relation.
s/a/the/

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode71
state/relation.go:71: // Role returns the role of a relation.
// RelationRole returns the service role within the relation.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode76
state/relation.go:76: // Name returns the name of a relation.
// RelationName returns the name this relation has within the service.

https://codereview.appspot.com/6223055/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode257
state/state.go:257: if relationKey != "" {
This is ignoring err. Even if it works, I'd prefer if we were more conservative
about it:

if _, ok := err.(*NoRelationError); ok {
    return false, nil
}
if err != nil {
    return false, err
}
return true, err

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode266
state/state.go:266: // addRelation creates the relation node depending on the
given scope.
I don't get what "depending on the given scope" is trying to tell me.

addRelation also seems like a bad name for this function. There's both
addRelation and AddRelation, and they are very different beasts.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode274
state/state.go:274: // Creation is per container for container scoped relations
and
I don't quite get what this comment is saying either.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode276
state/state.go:276: if scope == ScopeGlobal {
This seems weird. Why is it only introducing the settings if the relation is
global?  I don't know if that makes sense, and if it does we certainly need a
clear comment here explaining the background.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode286
state/state.go:286: // and endpoint.
s/and endpoint/endpoint/, comments above hold here and in the function below
too.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode300
state/state.go:300: // AddRelation creates a new relation state with the given
endpoints.
s/state//

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode305
state/state.go:305: return nil, nil, fmt.Errorf("state: counterpart for %s
endpoint is missing", endpoints[0].RelationRole)
"state: can't add non-peer relation with a single service"

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode309
state/state.go:309: return nil, nil, fmt.Errorf("state: endpoints %s and %s are
incompatible", endpoints[0], endpoints[1])
"state: can't add relation between %s and %s"

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode312
state/state.go:312: return nil, nil, fmt.Errorf("state: illegal number of
endpoints provided")
"state: can't add relations between %d services", len(endpoints)

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode314
state/state.go:314: // Check if the relation already exist.
s/exist/&s/

Also, this comment is misplaced. The line below is reading the topology.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode315
state/state.go:315: top, err := readTopology(s.zk)
"top" is a word in English, which is a bit confusing. How's the topology
variable named everywhere else? I suggest preserving it (it'd be fine to rename
everything to "topo", though).

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode319
state/state.go:319: alreadyAdded, err := s.hasRelation(top, endpoints...)
This is the only use of this function, which means that both its name and its
organization may be tailored for the use that actually makes sense. As it is,
it's transforming the result of RelationKey into some values, which are then
returned back to AddRelation, which is then processed once more below to produce
the error message that is what is really useful in the first place.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode324
state/state.go:324: return nil, nil, fmt.Errorf("state: relation has already
been added")
/has already been added/already exists/

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode326
state/state.go:326: // Add relation and service relations.
Comment needs love.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode334
state/state.go:334: relationKey, err := s.addRelation(scope)
Have we verified that the endpoints make sense at this point? It looks like
we've checked that there is a good number of them, but I can't see it verifying
that the endpoints make sense (service names, relation names, interface names,
etc). It'd be bad to be creating state in ZooKeeper just to later tell something
trivial to the user that we could have verified upfront. We shouldn't be
duplicating this logic, though.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode369
state/state.go:369: Service:      serviceRelation.serviceKey,
serviceRelation has a public API that we can use it here, rather than fiddling
with its internals assuming we know what's going on.

Same below.

https://codereview.appspot.com/6223055/diff/9001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/state_test.go#newcode1027
state/state_test.go:1027: c.Assert(relation, NotNil)
c.Assert(serviceRelations, HasLen, 2)

This will below up in a helpful way, with proper debugging information, in case
that's not true (it'll panic ATM).

[TheMue, 2012-05-31 16:23:59]

Please take a look.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode50
state/relation.go:50: // Relation represents a connection between one or more
services.
On 2012/05/30 22:35:42, niemeyer wrote:
> // Relation represents a link between services, or within a
> // service (in the case of peer relations).

Done.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode56
state/relation.go:56: // ServiceRelation represents a relation between one or
more services.
On 2012/05/30 22:35:42, niemeyer wrote:
> // ServiceRelation represents an established relation from
> // the viewpoint of a participant service.

Done.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode59
state/relation.go:59: key        string
On 2012/05/30 22:35:42, niemeyer wrote:
> s/key/relationKey/; given the type name, this would be ambiguous.

Done.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode63
state/relation.go:63: name       string
On 2012/05/30 22:35:42, niemeyer wrote:
> relationScope, relationRole, relationName, for the same reason.
> 
> Also on the methods below please (RelationRole, RelationName, etc).

Done.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode66
state/relation.go:66: // Scope returns the scope of a relation.
On 2012/05/30 22:35:42, niemeyer wrote:
> s/a/the/

Done.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode71
state/relation.go:71: // Role returns the role of a relation.
On 2012/05/30 22:35:42, niemeyer wrote:
> // RelationRole returns the service role within the relation. 

Done.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode76
state/relation.go:76: // Name returns the name of a relation.
On 2012/05/30 22:35:42, niemeyer wrote:
> // RelationName returns the name this relation has within the service.

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode257
state/state.go:257: if relationKey != "" {
On 2012/05/30 22:35:42, niemeyer wrote:
> This is ignoring err. Even if it works, I'd prefer if we were more
conservative
> about it:
> 
> if _, ok := err.(*NoRelationError); ok {
>     return false, nil
> }
> if err != nil {
>     return false, err
> }
> return true, err

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode266
state/state.go:266: // addRelation creates the relation node depending on the
given scope.
On 2012/05/30 22:35:42, niemeyer wrote:
> I don't get what "depending on the given scope" is trying to tell me.
> 
> addRelation also seems like a bad name for this function. There's both
> addRelation and AddRelation, and they are very different beasts.

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode274
state/state.go:274: // Creation is per container for container scoped relations
and
On 2012/05/30 22:35:42, niemeyer wrote:
> I don't quite get what this comment is saying either.

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode276
state/state.go:276: if scope == ScopeGlobal {
On 2012/05/30 22:35:42, niemeyer wrote:
> This seems weird. Why is it only introducing the settings if the relation is
> global?  I don't know if that makes sense, and if it does we certainly need a
> clear comment here explaining the background.

I tried to make the comments more clear, they have been almost a 1:1 copy of the
Python code. We should talk about the container scope and if the whole method(s)
can't be modified to handle this too. So far I don't know enough about the
meaning of the scope and have to analyze it first.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode286
state/state.go:286: // and endpoint.
On 2012/05/30 22:35:42, niemeyer wrote:
> s/and endpoint/endpoint/, comments above hold here and in the function below
> too.

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode300
state/state.go:300: // AddRelation creates a new relation state with the given
endpoints.
On 2012/05/30 22:35:42, niemeyer wrote:
> s/state//

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode305
state/state.go:305: return nil, nil, fmt.Errorf("state: counterpart for %s
endpoint is missing", endpoints[0].RelationRole)
On 2012/05/30 22:35:42, niemeyer wrote:
> "state: can't add non-peer relation with a single service"

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode309
state/state.go:309: return nil, nil, fmt.Errorf("state: endpoints %s and %s are
incompatible", endpoints[0], endpoints[1])
On 2012/05/30 22:35:42, niemeyer wrote:
> "state: can't add relation between %s and %s"

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode312
state/state.go:312: return nil, nil, fmt.Errorf("state: illegal number of
endpoints provided")
On 2012/05/30 22:35:42, niemeyer wrote:
> "state: can't add relations between %d services", len(endpoints)

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode314
state/state.go:314: // Check if the relation already exist.
On 2012/05/30 22:35:42, niemeyer wrote:
> s/exist/&s/
> 
> Also, this comment is misplaced. The line below is reading the topology.

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode315
state/state.go:315: top, err := readTopology(s.zk)
On 2012/05/30 22:35:42, niemeyer wrote:
> "top" is a word in English, which is a bit confusing. How's the topology
> variable named everywhere else? I suggest preserving it (it'd be fine to
rename
> everything to "topo", though).

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode319
state/state.go:319: alreadyAdded, err := s.hasRelation(top, endpoints...)
On 2012/05/30 22:35:42, niemeyer wrote:
> This is the only use of this function, which means that both its name and its
> organization may be tailored for the use that actually makes sense. As it is,
> it's transforming the result of RelationKey into some values, which are then
> returned back to AddRelation, which is then processed once more below to
produce
> the error message that is what is really useful in the first place.

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode324
state/state.go:324: return nil, nil, fmt.Errorf("state: relation has already
been added")
On 2012/05/30 22:35:42, niemeyer wrote:
> /has already been added/already exists/

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode324
state/state.go:324: return nil, nil, fmt.Errorf("state: relation has already
been added")
On 2012/05/30 22:35:42, niemeyer wrote:
> /has already been added/already exists/

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode326
state/state.go:326: // Add relation and service relations.
On 2012/05/30 22:35:42, niemeyer wrote:
> Comment needs love.

Done.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode334
state/state.go:334: relationKey, err := s.addRelation(scope)
On 2012/05/30 22:35:42, niemeyer wrote:
> Have we verified that the endpoints make sense at this point? It looks like
> we've checked that there is a good number of them, but I can't see it
verifying
> that the endpoints make sense (service names, relation names, interface names,
> etc). It'd be bad to be creating state in ZooKeeper just to later tell
something
> trivial to the user that we could have verified upfront. We shouldn't be
> duplicating this logic, though.

Today this isn't done. I would like to do it in a new small branch after this
and RemoveRelation() are in.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode369
state/state.go:369: Service:      serviceRelation.serviceKey,
On 2012/05/30 22:35:42, niemeyer wrote:
> serviceRelation has a public API that we can use it here, rather than fiddling
> with its internals assuming we know what's going on.
> 
> Same below.

The name can be retrieved with RelationName() but the service key is internal so
far. Shall a public accessor be added?

https://codereview.appspot.com/6223055/diff/9001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/state_test.go#newcode1027
state/state_test.go:1027: c.Assert(relation, NotNil)
On 2012/05/30 22:35:42, niemeyer wrote:
> c.Assert(serviceRelations, HasLen, 2)
> 
> This will below up in a helpful way, with proper debugging information, in
case
> that's not true (it'll panic ATM).

Done.

[niemeyer, 2012-06-01 11:58:12]

The code diff has a lot of unrelated code. Please let me know once the code is
clean and may be reviewed.

[TheMue, 2012-06-01 12:05:05]

Please take a look.

[TheMue, 2012-06-01 12:06:36]

Please take a look.

[niemeyer, 2012-06-01 12:21:52]

LGTM, but see the comments please:

https://codereview.appspot.com/6223055/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode276
state/state.go:276: if scope == ScopeGlobal {
On 2012/05/31 16:23:59, TheMue wrote:
> On 2012/05/30 22:35:42, niemeyer wrote:
> > This seems weird. Why is it only introducing the settings if the relation is
> > global?  I don't know if that makes sense, and if it does we certainly need
a
> > clear comment here explaining the background.
> 
> I tried to make the comments more clear, they have been almost a 1:1 copy of
the
> Python code. We should talk about the container scope and if the whole
method(s)
> can't be modified to handle this too. So far I don't know enough about the
> meaning of the scope and have to analyze it first.

Okay, this branch sounds fine besides what we already talked about and the minor
points, but please do understand the scope logic. As you have seen in the
relation+topology story, writing logic when you have no idea of what is going on
is hard.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode334
state/state.go:334: relationKey, err := s.addRelation(scope)
On 2012/05/31 16:23:59, TheMue wrote:
> On 2012/05/30 22:35:42, niemeyer wrote:
> > Have we verified that the endpoints make sense at this point? It looks like
> > we've checked that there is a good number of them, but I can't see it
> verifying
> > that the endpoints make sense (service names, relation names, interface
names,
> > etc). It'd be bad to be creating state in ZooKeeper just to later tell
> something
> > trivial to the user that we could have verified upfront. We shouldn't be
> > duplicating this logic, though.
> 
> Today this isn't done. I would like to do it in a new small branch after this
> and RemoveRelation() are in.

I filed a bug to keep track of this and assigned to you:

https://bugs.launchpad.net/juju-core/+bug/1007373

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode369
state/state.go:369: Service:      serviceRelation.serviceKey,
On 2012/05/31 16:23:59, TheMue wrote:
> On 2012/05/30 22:35:42, niemeyer wrote:
> > serviceRelation has a public API that we can use it here, rather than
fiddling
> > with its internals assuming we know what's going on.
> > 
> > Same below.
> 
> The name can be retrieved with RelationName() but the service key is internal
so
> far. Shall a public accessor be added?

No, you're right. serviceKey is internal.

https://codereview.appspot.com/6223055/diff/10005/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6223055/diff/10005/state/state.go#newcode263
state/state.go:263: // container occurs in ServiceRelation.AddUnit().
That's a lot nicer, thanks.

Would you mind to just drop the () as we tend to refer to methods by their name
without the call syntax.

https://codereview.appspot.com/6223055/diff/10005/state/state.go#newcode279
state/state.go:279: if scope == ScopeGlobal {
That's a bit strange. This method only does anything at all if the scope is not
global, and the scope is only provided so it can be checked here for not doing
anything at all.

Instead, we can drop the test here, drop the parameter, and check at the scope
at the call site. That makes both this function and the call site more clear.

https://codereview.appspot.com/6223055/diff/10005/state/state.go#newcode294
state/state.go:294: return nil, nil, fmt.Errorf("state: can't add non-peer
relation with a single service")
Can you please drop all the "state:" prefixes being _added_ in this branch.
After much debate, we'll start dropping those prefixes and trusting more on the
message itself. Among other reasons, the prefix looks really bad once they're
compounded ("pkg1: foo bar baz: pkg2: blah blah: pkg4: etc").

Don't worry about previously existing prefixes. We'll cover these by themselves
in an isolated branch.

[TheMue, 2012-06-01 12:41:39]

Please take a look.

https://codereview.appspot.com/6223055/diff/10005/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6223055/diff/10005/state/state.go#newcode263
state/state.go:263: // container occurs in ServiceRelation.AddUnit().
On 2012/06/01 12:21:53, niemeyer wrote:
> That's a lot nicer, thanks.
> 
> Would you mind to just drop the () as we tend to refer to methods by their
name
> without the call syntax.

Done.

https://codereview.appspot.com/6223055/diff/10005/state/state.go#newcode279
state/state.go:279: if scope == ScopeGlobal {
On 2012/06/01 12:21:53, niemeyer wrote:
> That's a bit strange. This method only does anything at all if the scope is
not
> global, and the scope is only provided so it can be checked here for not doing
> anything at all.
> 
> Instead, we can drop the test here, drop the parameter, and check at the scope
> at the call site. That makes both this function and the call site more clear.

Done.

[TheMue, 2012-06-04 14:51:07]

*** Submitted:

state: Added two methods to add relations to State.

AddClientServerReleation() gets a client and a server endpoint,
AddPeerRelation() a peer endpoint. They return the relation and
both service relations for client/server or the one for peer.
The seperation into two methods with well defined arguments make
it easier to ensure a valid relation creation.

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