Issue 6200044: Added relations to topology. (Closed)
DescriptionAdded relations to topology.
The topology now supports relations. The methods
handling endpoints are not yet implemented.
https://code.launchpad.net/~themue/juju/go-state-topology-relations-without-endpoints/+merge/104765
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : Added relations to topology. #
Total comments: 32
Patch Set 3 : Added relations to topology. #
Total comments: 15
Patch Set 4 : Added relations to topology. #Patch Set 5 : Added relations to topology. #
Total comments: 2
Patch Set 6 : Added relations to topology. #
Total comments: 27
Patch Set 7 : Added relations to topology. #
Total comments: 4
Patch Set 8 : Added relations to topology. #
Total comments: 31
Patch Set 9 : Added relations to topology. #Patch Set 10 : Added relations to topology. #
Total comments: 2
Patch Set 11 : Added relations to topology. #Patch Set 12 : Added relations to topology. #
MessagesTotal messages: 21
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
These are just preliminary thoughts, really; let me know what you think. https://codereview.appspot.com/6200044/diff/3001/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/3001/state/relation.go#newcode26 state/relation.go:26: ServiceName string `yaml:"service_name"` I have a vague feeling we prefer - to _ in such situations, for consistency's sake. https://codereview.appspot.com/6200044/diff/3001/state/relation.go#newcode33 state/relation.go:33: // MayRelateTo tests whether the "other"`" endpoint may be used in a common I *think* "CanRelateTo" might work better. Other opinions? https://codereview.appspot.com/6200044/diff/3001/state/relation.go#newcode40 state/relation.go:40: func (e *RelationEndpoint) MayRelateTo(other *RelationEndpoint) bool { I'd find it easier to follow a less nested approach here: if e.RelationType != other.RelationType { return false } if e.RelationType == RolePeer { return other.RelationType == RolePeer } ...etc https://codereview.appspot.com/6200044/diff/3001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode23 state/topology.go:23: Relations map[string]*zkRelation I'm not sure "zk" is the right prefix... surely it's purely about the topology, and the fact that the topology node *happens* to be stored in ZK (like everything else ;)) doesn't feel very significant here. "tpRelation", perhaps? This probably shouldn't be changed in this branch, though, because it'll hit several types. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode45 state/topology.go:45: // zkRelations represents the relation data within the s/zkRelations/zkRelation https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode54 state/topology.go:54: // a relation within the /topology node in ZooKeeper. I don't think this is a "settings" at all... https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode381 state/topology.go:381: // RelationServices returns all services associated to the relation. s/associated to/a member of/ ? https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode406 state/topology.go:406: // assigned to 'relationKey'. s/assigned to/a member of/ ? https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode424 state/topology.go:424: // AssignServiceToRelation associates a service to a relation. s/associates/adds/ https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode425 state/topology.go:425: // Relation role and name have to be passed. Can't we figure out name, given serviceKey? https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode441 state/topology.go:441: } Should we maybe also also check that we're not doing anything obviously insane like mixing peer roles with client/server roles within the relation? https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode448 state/topology.go:448: func (t *topology) UnassignServiceFromRelation(relationKey, serviceKey string) error { We don't use this anywhere in the python. If we ever turn out to need it, we can write it then. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode464 state/topology.go:464: // associated service. It'd be good to know a bit more about how you intend to use this; it seems that the methods returning relationInfos correspond to Python methods that return slightly different data. (It may well be perfectly sensible, but I don't quite follow atm) https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode470 state/topology.go:470: Name string ServiceName? https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode475 state/topology.go:475: func (t *topology) RelationServiceSettings(relationKey, serviceKey string) (*relationInfo, error) { I don't think the word "settings" is right here either, especially if we're returning a relationInfo.
Sign in to reply to this message.
Just another note, sorry I missed it before. https://codereview.appspot.com/6200044/diff/3001/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/3001/state/relation.go#newcode28 state/relation.go:28: RelationName string `yaml:"relation_name"` I don't think this is meaningful -- (1) relations don't really have human-readable names in the first place, and (2) we want to use CanRelateTo *before* we actually create a relation between endpoints in the first place.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6200044/diff/3001/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/3001/state/relation.go#newcode26 state/relation.go:26: ServiceName string `yaml:"service_name"` On 2012/05/09 00:44:29, fwereade wrote: > I have a vague feeling we prefer - to _ in such situations, for consistency's > sake. Indeed, too quickly taken the underscore. https://codereview.appspot.com/6200044/diff/3001/state/relation.go#newcode28 state/relation.go:28: RelationName string `yaml:"relation_name"` On 2012/05/09 18:19:56, fwereade wrote: > I don't think this is meaningful -- (1) relations don't really have > human-readable names in the first place, and (2) we want to use CanRelateTo > *before* we actually create a relation between endpoints in the first place. Done. https://codereview.appspot.com/6200044/diff/3001/state/relation.go#newcode33 state/relation.go:33: // MayRelateTo tests whether the "other"`" endpoint may be used in a common On 2012/05/09 00:44:29, fwereade wrote: > I *think* "CanRelateTo" might work better. Other opinions? Done. https://codereview.appspot.com/6200044/diff/3001/state/relation.go#newcode40 state/relation.go:40: func (e *RelationEndpoint) MayRelateTo(other *RelationEndpoint) bool { On 2012/05/09 00:44:29, fwereade wrote: > I'd find it easier to follow a less nested approach here: > > if e.RelationType != other.RelationType { > return false > } > if e.RelationType == RolePeer { > return other.RelationType == RolePeer > } > ...etc Done. https://codereview.appspot.com/6200044/diff/3001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode23 state/topology.go:23: Relations map[string]*zkRelation On 2012/05/09 00:44:29, fwereade wrote: > I'm not sure "zk" is the right prefix... surely it's purely about the topology, > and the fact that the topology node *happens* to be stored in ZK (like > everything else ;)) doesn't feel very significant here. "tpRelation", perhaps? > > This probably shouldn't be changed in this branch, though, because it'll hit > several types. As discussed it's a good idea to exchange it to a technology independent prefix like "top" in a later branch. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode45 state/topology.go:45: // zkRelations represents the relation data within the On 2012/05/09 00:44:29, fwereade wrote: > s/zkRelations/zkRelation Done. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode54 state/topology.go:54: // a relation within the /topology node in ZooKeeper. On 2012/05/09 00:44:29, fwereade wrote: > I don't think this is a "settings" at all... Change it to the discussed approach storing service keys in zkRelation in fields describing their role. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode381 state/topology.go:381: // RelationServices returns all services associated to the relation. On 2012/05/09 00:44:29, fwereade wrote: > s/associated to/a member of/ > > ? Done. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode406 state/topology.go:406: // assigned to 'relationKey'. On 2012/05/09 00:44:29, fwereade wrote: > s/assigned to/a member of/ > > ? Done. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode424 state/topology.go:424: // AssignServiceToRelation associates a service to a relation. On 2012/05/09 00:44:29, fwereade wrote: > s/associates/adds/ Done. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode425 state/topology.go:425: // Relation role and name have to be passed. On 2012/05/09 00:44:29, fwereade wrote: > Can't we figure out name, given serviceKey? It's gone with the redesign of zkRelation. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode441 state/topology.go:441: } On 2012/05/09 00:44:29, fwereade wrote: > Should we maybe also also check that we're not doing anything obviously insane > like mixing peer roles with client/server roles within the relation? As discussed may be obsolete with new AddRelation() approach. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode448 state/topology.go:448: func (t *topology) UnassignServiceFromRelation(relationKey, serviceKey string) error { On 2012/05/09 00:44:29, fwereade wrote: > We don't use this anywhere in the python. If we ever turn out to need it, we can > write it then. Done. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode464 state/topology.go:464: // associated service. On 2012/05/09 00:44:29, fwereade wrote: > It'd be good to know a bit more about how you intend to use this; it seems that > the methods returning relationInfos correspond to Python methods that return > slightly different data. (It may well be perfectly sensible, but I don't quite > follow atm) It's now "endpointInfo" and contains the service key instead of its name. https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode470 state/topology.go:470: Name string On 2012/05/09 00:44:29, fwereade wrote: > ServiceName? Done. Now "ServiceKey" containing the key. "Key" is renamed to "RelationKey". https://codereview.appspot.com/6200044/diff/3001/state/topology.go#newcode475 state/topology.go:475: func (t *topology) RelationServiceSettings(relationKey, serviceKey string) (*relationInfo, error) { On 2012/05/09 00:44:29, fwereade wrote: > I don't think the word "settings" is right here either, especially if we're > returning a relationInfo. Changed this and the following methods to talk about "endpointInfos". It's now named "ActiveServiceEndpoint()", the one below "ActiveServiceEndpoints()".
Sign in to reply to this message.
https://codereview.appspot.com/6200044/diff/10001/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/10001/state/relation.go#newcode27 state/relation.go:27: RelationType string `yaml:"relation-type"` Given that endpointInfo uses Interface rather than RelationType, I'd prefer that this also be consistent with that (and ofc with the public-facing terminology in charms). https://codereview.appspot.com/6200044/diff/10001/state/relation.go#newcode51 state/relation.go:51: return false This feels like a panic() situation to me https://codereview.appspot.com/6200044/diff/10001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode48 state/topology.go:48: // combination (server/client or peer) has to be validated. Update to refer to Members https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode350 state/topology.go:350: // the given type. AddRelation and AssignServiceToRelation don't quite work for me, primarily because they can be used wrong and lead to us storing crack in the topology. If we replace them with AddPeerRelation and AddClientServerRelation we'll (1) eliminate a whole class of "used API in crazy fashion" errors; (2) be able to skip a lot of the combinatorial validation that I'd otherwise be asking us to add to Assign...; and (3) have just the same number of methods, probably adding up to less code. (Certainly less code than we'd have if we did properly careful validation in Assign...). https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode427 state/topology.go:427: func (t *topology) AssignServiceToRelation(relationKey, serviceKey string, role RelationRole) error { See above. https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode448 state/topology.go:448: // endpointInfo bundles the informations of an endpoint s/informations/information/ https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode458 state/topology.go:458: // ActiveServiceEndpoint returns informations about the endpoint s/informations/information/ https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode482 state/topology.go:482: // ActiveServiceEndpoints returns all informations of the endpoints s/all informations of/information about all/
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6200044/diff/10001/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/10001/state/relation.go#newcode27 state/relation.go:27: RelationType string `yaml:"relation-type"` On 2012/05/10 00:19:30, fwereade wrote: > Given that endpointInfo uses Interface rather than RelationType, I'd prefer that > this also be consistent with that (and ofc with the public-facing terminology in > charms). Done. https://codereview.appspot.com/6200044/diff/10001/state/relation.go#newcode51 state/relation.go:51: return false On 2012/05/10 00:19:30, fwereade wrote: > This feels like a panic() situation to me Done. https://codereview.appspot.com/6200044/diff/10001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode48 state/topology.go:48: // combination (server/client or peer) has to be validated. On 2012/05/10 00:19:30, fwereade wrote: > Update to refer to Members Done.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6200044/diff/10001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode350 state/topology.go:350: // the given type. On 2012/05/10 00:19:30, fwereade wrote: > AddRelation and AssignServiceToRelation don't quite work for me, primarily > because they can be used wrong and lead to us storing crack in the topology. If > we replace them with AddPeerRelation and AddClientServerRelation we'll (1) > eliminate a whole class of "used API in crazy fashion" errors; (2) be able to > skip a lot of the combinatorial validation that I'd otherwise be asking us to > add to Assign...; and (3) have just the same number of methods, probably adding > up to less code. (Certainly less code than we'd have if we did properly careful > validation in Assign...). Done. https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode448 state/topology.go:448: // endpointInfo bundles the informations of an endpoint On 2012/05/10 00:19:30, fwereade wrote: > s/informations/information/ Done. https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode458 state/topology.go:458: // ActiveServiceEndpoint returns informations about the endpoint On 2012/05/10 00:19:30, fwereade wrote: > s/informations/information/ Done. https://codereview.appspot.com/6200044/diff/10001/state/topology.go#newcode482 state/topology.go:482: // ActiveServiceEndpoints returns all informations of the endpoints On 2012/05/10 00:19:30, fwereade wrote: > s/all informations of/information about all/ Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6200044/diff/14001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/14001/state/topology.go#newcode350 state/topology.go:350: // the server. It will get the given key, interface and scope. "It will get"?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6200044/diff/14001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/14001/state/topology.go#newcode350 state/topology.go:350: // the server. It will get the given key, interface and scope. On 2012/05/11 19:18:58, fwereade wrote: > "It will get"? Done.
Sign in to reply to this message.
https://codereview.appspot.com/6200044/diff/3003/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/3003/state/relation.go#newcode10 state/relation.go:10: RoleServer RelationRole = "server" Maybe we should take the chance to rename this to RoleProvider and RoleRequirer. This is the real meaning of those roles. They don't have to be clients or servers. https://codereview.appspot.com/6200044/diff/3003/state/relation.go#newcode28 state/relation.go:28: RelationRole RelationRole `yaml:"relation-role"` Why the yaml? https://codereview.appspot.com/6200044/diff/3003/state/relation.go#newcode29 state/relation.go:29: RelationScope RelationScope `yaml:"relation-scope"` Where is the relation name? The endpoint is ambiguous without a relation name (which of ServiceName's relations is this talking about?). https://codereview.appspot.com/6200044/diff/3003/state/relation.go#newcode50 state/relation.go:50: panic("invalid endpoint role") This makes more sense given that RoleNone exists: panic("endpoint role is undefined") https://codereview.appspot.com/6200044/diff/3003/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode52 state/topology.go:52: Members map[RelationRole]string The Python naming feels more clear here: s/Members/Services/ https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode345 state/topology.go:345: func (t *topology) HasRelation(key string) bool { I suggest killing pretty much all of the functions below, renaming zkRelation to simply "relation", and then defining these functions: func (t) AddRelation(relationKey string, rel relation) error func (t) Relation(relationKey string) (relation, error) https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode351 state/topology.go:351: func (t *topology) AddClientServerRelation(relationKey, clientKey, serverKey, ifce string, scope RelationScope) error { Covered by AddRelation(key, rel). https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode379 state/topology.go:379: func (t *topology) AddPeerRelation(relationKey, peerKey, ifce string, scope RelationScope) error { Covered by AddRelation(key, rel) https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode411 state/topology.go:411: func (t *topology) RelationServices(key string) (map[string]RelationRole, error) { Covered by Relation(key). https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode426 state/topology.go:426: func (t *topology) RelationInterface(key string) (string, error) { Covered by Relation(key). https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode434 state/topology.go:434: func (t *topology) RelationScope(key string) (RelationScope, error) { Covered by Relation(key). https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode443 state/topology.go:443: func (t *topology) RelationHasService(relationKey, serviceKey string) bool { Covered by Relation(key) https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode461 state/topology.go:461: type endpointInfo struct { Endpoint looks wrong here too. It doesn't make sense for an endpoint to have a relation key. In fact, why do we have another endpoint definition here? Can't these functions operate with a RelationEndpoint? https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode470 state/topology.go:470: // between a service and a relation. Why "Active"? Do we have inactive endpoints? Also, "endpoint between a service and a relation"!? These two methods need love.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6200044/diff/3003/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/3003/state/relation.go#newcode10 state/relation.go:10: RoleServer RelationRole = "server" On 2012/05/21 19:21:51, niemeyer wrote: > Maybe we should take the chance to rename this to RoleProvider and RoleRequirer. > This is the real meaning of those roles. They don't have to be clients or > servers. Done. But used RoleConsumer instead of RoleRequirer, IMHO provider and cosumer are a better pair. https://codereview.appspot.com/6200044/diff/3003/state/relation.go#newcode28 state/relation.go:28: RelationRole RelationRole `yaml:"relation-role"` On 2012/05/21 19:21:51, niemeyer wrote: > Why the yaml? Oops, when I first touched the endpoints I thought they would be stored in ZK as entity. Later forgot to remove it. Thx. https://codereview.appspot.com/6200044/diff/3003/state/relation.go#newcode29 state/relation.go:29: RelationScope RelationScope `yaml:"relation-scope"` On 2012/05/21 19:21:51, niemeyer wrote: > Where is the relation name? The endpoint is ambiguous without a relation name > (which of ServiceName's relations is this talking about?). Hmm, interpreted it wrong so that service plus interface are unique. Will add relation name. https://codereview.appspot.com/6200044/diff/3003/state/relation.go#newcode50 state/relation.go:50: panic("invalid endpoint role") On 2012/05/21 19:21:51, niemeyer wrote: > This makes more sense given that RoleNone exists: > > panic("endpoint role is undefined") Done. https://codereview.appspot.com/6200044/diff/3003/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode52 state/topology.go:52: Members map[RelationRole]string On 2012/05/21 19:21:51, niemeyer wrote: > The Python naming feels more clear here: s/Members/Services/ Done. https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode345 state/topology.go:345: func (t *topology) HasRelation(key string) bool { On 2012/05/21 19:21:51, niemeyer wrote: > I suggest killing pretty much all of the functions below, renaming zkRelation to > simply "relation", and then defining these functions: > > func (t) AddRelation(relationKey string, rel relation) error > func (t) Relation(relationKey string) (relation, error) > The renaming of zkRelation to relation almost matches an idea of William and me to rename all zk... types in topology to top... (in an extra branch). So the name isn't technology-bound and descibes that it's part of the topology. So if it's ok I would keep the name zkRelation until the renaming branch (could be done quickly after this queue is in). I like the idea of simplification. Error checking of the data off zkRelation has to be more careful (no mix of provider or consumer and peer) but that's ok. https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode351 state/topology.go:351: func (t *topology) AddClientServerRelation(relationKey, clientKey, serverKey, ifce string, scope RelationScope) error { On 2012/05/21 19:21:51, niemeyer wrote: > Covered by AddRelation(key, rel). Done. https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode379 state/topology.go:379: func (t *topology) AddPeerRelation(relationKey, peerKey, ifce string, scope RelationScope) error { On 2012/05/21 19:21:51, niemeyer wrote: > Covered by AddRelation(key, rel) Done. https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode411 state/topology.go:411: func (t *topology) RelationServices(key string) (map[string]RelationRole, error) { On 2012/05/21 19:21:51, niemeyer wrote: > Covered by Relation(key). Done. https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode426 state/topology.go:426: func (t *topology) RelationInterface(key string) (string, error) { On 2012/05/21 19:21:51, niemeyer wrote: > Covered by Relation(key). Done. https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode434 state/topology.go:434: func (t *topology) RelationScope(key string) (RelationScope, error) { On 2012/05/21 19:21:51, niemeyer wrote: > Covered by Relation(key). Done. https://codereview.appspot.com/6200044/diff/3003/state/topology.go#newcode443 state/topology.go:443: func (t *topology) RelationHasService(relationKey, serviceKey string) bool { On 2012/05/21 19:21:51, niemeyer wrote: > Covered by Relation(key) Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6200044/diff/3003/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/3003/state/relation.go#newcode10 state/relation.go:10: RoleServer RelationRole = "server" On 2012/05/22 16:35:26, TheMue wrote: > On 2012/05/21 19:21:51, niemeyer wrote: > > Maybe we should take the chance to rename this to RoleProvider and > RoleRequirer. > > This is the real meaning of those roles. They don't have to be clients or > > servers. > > Done. But used RoleConsumer instead of RoleRequirer, IMHO provider and cosumer > are a better pair. -1 on this. "provides" and "requires" are used in charms; IMO consistency is valuable here. https://codereview.appspot.com/6200044/diff/17002/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/17002/state/relation.go#newcode10 state/relation.go:10: RoleConsumer RelationRole = "consumer" RoleRequirer if possible as mentioned elsewhere https://codereview.appspot.com/6200044/diff/17002/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/17002/state/topology.go#newcode74 state/topology.go:74: return fmt.Errorf("too much services defined") s/much/many/
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6200044/diff/17002/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/17002/state/relation.go#newcode10 state/relation.go:10: RoleConsumer RelationRole = "consumer" On 2012/05/23 10:28:02, fwereade wrote: > RoleRequirer if possible as mentioned elsewhere Done. https://codereview.appspot.com/6200044/diff/17002/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/17002/state/topology.go#newcode74 state/topology.go:74: return fmt.Errorf("too much services defined") On 2012/05/23 10:28:02, fwereade wrote: > s/much/many/ Done.
Sign in to reply to this message.
Looks close. https://codereview.appspot.com/6200044/diff/21001/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/21001/state/relation.go#newcode31 state/relation.go:31: // CanRelateTo tests whether the "other"`" endpoint can be used in a common // CanRelateTo returns whether a relation may be established between e and other. Please drop the rest of the comment as it's misleading (what's a 'client' or a 'server'?) https://codereview.appspot.com/6200044/diff/21001/state/relation.go#newcode47 state/relation.go:47: return other.RelationRole == RolePeer I'm not sure this is right. Peer is an internal relation. It doesn't make sense for a service to be a peer with another service right now. Can you please drop this for the moment as well? https://codereview.appspot.com/6200044/diff/21001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode14 state/topology.go:14: Why the line between the comment and the constant being commented on? https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode47 state/topology.go:47: // /topology node in ZooKeeper. "Services" references to If there's a need to document a field, that should be next to the field itself in the struct. No quotes around the field name as well please (we use that convention across the board). https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode57 state/topology.go:57: // services are provider and consumer or peer. // check verifies that r is a proper relation. No need to describe what the code does. The description will likely go out of date very soon, and we can verify the logic easily below. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode62 state/topology.go:62: switch len(r.Services) { The logic below misbehaves in arbitrary ways when given unknown roles or roles with empty names. I suggest something along these lines: if len(r.Services) == 0 { return fmt.Errorf("relation has no services") } counterpart := map[RoleRelation]RoleRelation{ RoleRequirer: RoleProvider, RoleProvider: RoleRequirer, RolePeer: RolePeer, } for role1, _ := range r.Services { if name == "" { fmt.Errorf("relation has %s service with empty name1", role) } role2, ok := counterpart[role1] if !ok { fmt.Errorf("relation has unknown service role: %q", role1) } if _, ok := r.Services[role2]; !ok { fmt.Errorf("relation has %s but no %s", role1, role2) } } if len(r.Services) == 3 { return fmt.Errorf("relation with mixed peer, provider, and requirer roles") } https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode368 state/topology.go:368: // Relation returns a relation with the given key or an error if it doesn't exist. // Relation returns the relation with key from the topology. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode369 state/topology.go:369: func (t *topology) Relation(relationKey string) (*zkRelation, error) { s/relationKey/key/ (or change RemoveRelation and Service and the other methods that use "key" when that's unambiguous) https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode403 state/topology.go:403: relation.Key = relationKey This will cause the relation key to be persisted within the topology. I retract what I said. It was a bad idea to have Key in the relation because we'll invariably be making that kind of mistake. Can you please take it off and we'll sort out the other needs without it? https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode408 state/topology.go:408: // RelationKeys returns all relation keys. // RelationKeys returns the keys for all relations in the topology. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode418 state/topology.go:418: // RemoveRelation removes a relation. // RemoveRelation removes the relation with key from the topology. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode426 state/topology.go:426: func (t *topology) RelationWithService(relationKey, serviceKey string) (*zkRelation, error) { This method is a bit weird. Can we please leave it out for the moment? https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode443 state/topology.go:443: // given service key. // RelationsForService returns all relations that the service // with serviceKey is part of. I notice you struggle a little bit with "a" vs "the" in documentation. "the" is the definite article. When talking about a specific thing, we should use "the" thing, not "a" thing. Compare: "The window from the living room was open." This is addressing a specific window, well known. "A window from the house was open." It's undefined which window was open. Compare the suggested comment with the original one in that regard. We'll return all relations for *the service with serviceKey*. It's not just an arbitrary service with an arbitrary key. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode448 state/topology.go:448: relations := []*zkRelation{} We're taking Key out of zkRelation, so let's define this as: relations := make(map[string]*zkRelation) where the map key is the relation key. I think that's the only thing we have to fix with the missing Key field. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode452 state/topology.go:452: relations = append(relations, relation) break
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6200044/diff/21001/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/21001/state/relation.go#newcode31 state/relation.go:31: // CanRelateTo tests whether the "other"`" endpoint can be used in a common On 2012/05/23 22:56:06, niemeyer wrote: > // CanRelateTo returns whether a relation may be established between e and > other. > > Please drop the rest of the comment as it's misleading (what's a 'client' or a > 'server'?) Done. https://codereview.appspot.com/6200044/diff/21001/state/relation.go#newcode47 state/relation.go:47: return other.RelationRole == RolePeer On 2012/05/23 22:56:06, niemeyer wrote: > I'm not sure this is right. Peer is an internal relation. It doesn't make sense > for a service to be a peer with another service right now. Can you please drop > this for the moment as well? It's allowed today in endpoint.py. Maybe it's an error there too? https://codereview.appspot.com/6200044/diff/21001/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode14 state/topology.go:14: On 2012/05/23 22:56:06, niemeyer wrote: > Why the line between the comment and the constant being commented on? Done. Came with Rev 151, don't know why. Maybe a merge error?!? https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode47 state/topology.go:47: // /topology node in ZooKeeper. "Services" references to On 2012/05/23 22:56:06, niemeyer wrote: > If there's a need to document a field, that should be next to the field itself > in the struct. No quotes around the field name as well please (we use that > convention across the board). Done. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode57 state/topology.go:57: // services are provider and consumer or peer. On 2012/05/23 22:56:06, niemeyer wrote: > // check verifies that r is a proper relation. > > No need to describe what the code does. The description will likely go out of > date very soon, and we can verify the logic easily below. Done. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode62 state/topology.go:62: switch len(r.Services) { On 2012/05/23 22:56:06, niemeyer wrote: > The logic below misbehaves in arbitrary ways when given unknown roles or roles > with empty names. I suggest something along these lines: > > if len(r.Services) == 0 { > return fmt.Errorf("relation has no services") > } > counterpart := map[RoleRelation]RoleRelation{ > RoleRequirer: RoleProvider, > RoleProvider: RoleRequirer, > RolePeer: RolePeer, > } > for role1, _ := range r.Services { > if name == "" { > fmt.Errorf("relation has %s service with empty name1", role) > } > role2, ok := counterpart[role1] > if !ok { > fmt.Errorf("relation has unknown service role: %q", role1) > } > if _, ok := r.Services[role2]; !ok { > fmt.Errorf("relation has %s but no %s", role1, role2) > } > } > if len(r.Services) == 3 { > return fmt.Errorf("relation with mixed peer, provider, and requirer roles") > } Done. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode368 state/topology.go:368: // Relation returns a relation with the given key or an error if it doesn't exist. On 2012/05/23 22:56:06, niemeyer wrote: > // Relation returns the relation with key from the topology. Done. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode369 state/topology.go:369: func (t *topology) Relation(relationKey string) (*zkRelation, error) { On 2012/05/23 22:56:06, niemeyer wrote: > s/relationKey/key/ > > (or change RemoveRelation and Service and the other methods that use "key" when > that's unambiguous) Done. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode403 state/topology.go:403: relation.Key = relationKey On 2012/05/23 22:56:06, niemeyer wrote: > This will cause the relation key to be persisted within the topology. I retract > what I said. It was a bad idea to have Key in the relation because we'll > invariably be making that kind of mistake. Can you please take it off and we'll > sort out the other needs without it? Done. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode408 state/topology.go:408: // RelationKeys returns all relation keys. On 2012/05/23 22:56:06, niemeyer wrote: > // RelationKeys returns the keys for all relations in the topology. Done. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode418 state/topology.go:418: // RemoveRelation removes a relation. On 2012/05/23 22:56:06, niemeyer wrote: > // RemoveRelation removes the relation with key from the topology. Done. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode426 state/topology.go:426: func (t *topology) RelationWithService(relationKey, serviceKey string) (*zkRelation, error) { On 2012/05/23 22:56:06, niemeyer wrote: > This method is a bit weird. Can we please leave it out for the moment? It's todays get_relation_service(). Maybe only the name is misleading. But I've also be unhappy with the current namen. You've seen my first try to find a better one. How about func (t) ServiceRelation(serviceKey, relationKey string) (*zkRelation, err) for this one and func (t) ServiceRelations(serviceKey string) ([]*zkRelation, err) for the following one? https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode443 state/topology.go:443: // given service key. On 2012/05/23 22:56:06, niemeyer wrote: > // RelationsForService returns all relations that the service > // with serviceKey is part of. > > I notice you struggle a little bit with "a" vs "the" in documentation. "the" is > the definite article. When talking about a specific thing, we should use "the" > thing, not "a" thing. Compare: > > "The window from the living room was open." > > This is addressing a specific window, well known. > > "A window from the house was open." > > It's undefined which window was open. > > Compare the suggested comment with the original one in that regard. We'll return > all relations for *the service with serviceKey*. It's not just an arbitrary > service with an arbitrary key. Done, thx. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode448 state/topology.go:448: relations := []*zkRelation{} On 2012/05/23 22:56:06, niemeyer wrote: > We're taking Key out of zkRelation, so let's define this as: > > relations := make(map[string]*zkRelation) > > where the map key is the relation key. I think that's the only thing we have to > fix with the missing Key field. Done. https://codereview.appspot.com/6200044/diff/21001/state/topology.go#newcode452 state/topology.go:452: relations = append(relations, relation) On 2012/05/23 22:56:06, niemeyer wrote: > break Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
LGTM with the following two details sorted. Thanks! https://codereview.appspot.com/6200044/diff/21001/state/relation.go File state/relation.go (right): https://codereview.appspot.com/6200044/diff/21001/state/relation.go#newcode47 state/relation.go:47: return other.RelationRole == RolePeer On 2012/05/24 15:17:16, TheMue wrote: > On 2012/05/23 22:56:06, niemeyer wrote: > > I'm not sure this is right. Peer is an internal relation. It doesn't make > sense > > for a service to be a peer with another service right now. Can you please drop > > this for the moment as well? > > It's allowed today in endpoint.py. Maybe it's an error there too? <niemeyer> TheMue: I believe it is.. can you please drop it so that we can be sure about what's going in? If we find issues, it'll be a nice insight https://codereview.appspot.com/6200044/diff/26002/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/26002/state/topology.go#newcode46 state/topology.go:46: // /topology node in ZooKeeper.s s/\.s/./
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6200044/diff/26002/state/topology.go File state/topology.go (right): https://codereview.appspot.com/6200044/diff/26002/state/topology.go#newcode46 state/topology.go:46: // /topology node in ZooKeeper.s On 2012/05/24 17:47:16, niemeyer wrote: > s/\.s/./ Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted: Added relations to topology. The topology now supports relations. The methods handling endpoints are not yet implemented. R=fwereade, niemeyer CC= https://codereview.appspot.com/6200044
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
