state: introduce relation Scope concept

state: introduce relation Scope concept

RelationScopeWatcher will be used by RelationUnitsWatcher to determine what
presence keys should be watched for join/depart (and what settings keys
should then be watched when joined).

https://code.launchpad.net/~fwereade/juju-core/mstate-watch-relation-scope/+merge/125211

(do not edit description out of merge proposal)

[fwereade, 2012-09-19 13:51:37]

Please take a look.

[aram, 2012-09-19 14:25:34]

LGTM

[fwereade, 2012-09-19 14:29:35]

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go
File mstate/watcher.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go#newcode610
mstate/watcher.go:610: log.Printf("tick %#v", c)
whoops, sorry

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go#newcode624
mstate/watcher.go:624: log.Printf("tock %#v", c)
whoops, sorry

[aram, 2012-09-19 14:30:34]

> mstate/watcher.go:610: log.Printf("tick %#v", c)
> whoops, sorry

Strange, I thought that was a feature.

[niemeyer, 2012-09-19 15:43:33]

Looks surprisingly straightforward.

https://codereview.appspot.com/6533052/diff/1/mstate/open.go
File mstate/open.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/open.go#newcode92
mstate/open.go:92: relationRefs: db.C("relationRefs"),
I'd prefer to keep collection names lowercased, if that's ok. I *think* that's a
more common convention with Mongo.

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go
File mstate/relation.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode222
mstate/relation.go:222: // EnsureJoin ensures that the unit's relation settings
contain the expected
This documentation is detailing the implementation. It should cover what it
means for the consumer of the API instead.

Just as an analogy, imagine:

// Now calls the runtime package to obtain the number of
// nanoseconds since epoch and then returns a Time structure
// initialized with it.

vs.

// Now returns the current time.

The current comment can be used within the function, though, to describe the
internal details.

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode253
mstate/relation.go:253: // See EnsureJoin.
Ditto regarding comment.

FWIW, I think the "See" note isn't necessary. Those will be right next to each
other in the documentation.

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode271
mstate/relation.go:271: scope := strings.Join([]string{ru.scope, string(role)},
"#")
This is cheaper and more readable:

ru.scope + "#" + string(role)

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode323
mstate/relation.go:323: return strings.Join(parts, "#"), nil
FWIW, this is still not reflecting what we discussed today.

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode332
mstate/relation.go:332: func (d *relationRefDoc) UnitName() string {
unitName?

https://codereview.appspot.com/6533052/diff/1/mstate/relation_test.go
File mstate/relation_test.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/relation_test.go#newcode491
mstate/relation_test.go:491: case <-time.After(100 * time.Millisecond):
s/100/500/ (it's the failure side, so it's fine).

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go
File mstate/watcher.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go#newcode82
mstate/watcher.go:82: Added   []string
Feels a bit awkward to have low-level mangled strings here. There's nothing in
the API about those.

[fwereade, 2012-09-19 16:07:16]

just a couple of comments, GTWing on the rest

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go
File mstate/relation.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode323
mstate/relation.go:323: return strings.Join(parts, "#"), nil
On 2012/09/19 15:43:33, niemeyer wrote:
> FWIW, this is still not reflecting what we discussed today.

The "r#" is tacked onto the start of the RU's scope when it's created.

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go
File mstate/watcher.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go#newcode82
mstate/watcher.go:82: Added   []string
On 2012/09/19 15:43:33, niemeyer wrote:
> Feels a bit awkward to have low-level mangled strings here. There's nothing in
> the API about those.

This actually does just have unit names, although at least 40% of me wants to
use the icky names just for the convenience of the forthcoming RUW.

[niemeyer, 2012-09-19 16:22:43]

One more detail on the Sort.

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go
File mstate/watcher.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go#newcode82
mstate/watcher.go:82: Added   []string
On 2012/09/19 16:07:16, fwereade wrote:
> On 2012/09/19 15:43:33, niemeyer wrote:
> > Feels a bit awkward to have low-level mangled strings here. There's nothing
in
> > the API about those.
> 
> This actually does just have unit names, although at least 40% of me wants to
> use the icky names just for the convenience of the forthcoming RUW.

Uh, sorry. I've clearly read something that doesn't exist.

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go#newcode603
mstate/watcher.go:603: sort.Strings(changes.Removed)
The sorting here doesn't seem to make sense. Besides being wrong (unit/10 comes
before unit/2), the merge below spoils it even if it's correct.

[fwereade, 2012-09-19 16:57:05]

Please take a look.

https://codereview.appspot.com/6533052/diff/1/mstate/open.go
File mstate/open.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/open.go#newcode92
mstate/open.go:92: relationRefs: db.C("relationRefs"),
On 2012/09/19 15:43:33, niemeyer wrote:
> I'd prefer to keep collection names lowercased, if that's ok. I *think* that's
a
> more common convention with Mongo.

Done.

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go
File mstate/relation.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode222
mstate/relation.go:222: // EnsureJoin ensures that the unit's relation settings
contain the expected
On 2012/09/19 15:43:33, niemeyer wrote:
> This documentation is detailing the implementation. It should cover what it
> means for the consumer of the API instead.
> 
> Just as an analogy, imagine:
> 
> // Now calls the runtime package to obtain the number of
> // nanoseconds since epoch and then returns a Time structure
> // initialized with it.
> 
> vs.
> 
> // Now returns the current time.
> 
> The current comment can be used within the function, though, to describe the
> internal details.

Well put, thanks. Done.

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode253
mstate/relation.go:253: // See EnsureJoin.
On 2012/09/19 15:43:33, niemeyer wrote:
> Ditto regarding comment.
> 
> FWIW, I think the "See" note isn't necessary. Those will be right next to each
> other in the documentation.

Done.

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode271
mstate/relation.go:271: scope := strings.Join([]string{ru.scope, string(role)},
"#")
On 2012/09/19 15:43:33, niemeyer wrote:
> This is cheaper and more readable:
> 
> ru.scope + "#" + string(role)

Done.

https://codereview.appspot.com/6533052/diff/1/mstate/relation.go#newcode332
mstate/relation.go:332: func (d *relationRefDoc) UnitName() string {
On 2012/09/19 15:43:33, niemeyer wrote:
> unitName?

Done.

https://codereview.appspot.com/6533052/diff/1/mstate/relation_test.go
File mstate/relation_test.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/relation_test.go#newcode491
mstate/relation_test.go:491: case <-time.After(100 * time.Millisecond):
On 2012/09/19 15:43:33, niemeyer wrote:
> s/100/500/ (it's the failure side, so it's fine).

Done.

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go
File mstate/watcher.go (right):

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go#newcode82
mstate/watcher.go:82: Added   []string
On 2012/09/19 16:22:43, niemeyer wrote:
> On 2012/09/19 16:07:16, fwereade wrote:
> > On 2012/09/19 15:43:33, niemeyer wrote:
> > > Feels a bit awkward to have low-level mangled strings here. There's
nothing
> in
> > > the API about those.
> > 
> > This actually does just have unit names, although at least 40% of me wants
to
> > use the icky names just for the convenience of the forthcoming RUW.
> 
> Uh, sorry. I've clearly read something that doesn't exist.

No worries :)

https://codereview.appspot.com/6533052/diff/1/mstate/watcher.go#newcode603
mstate/watcher.go:603: sort.Strings(changes.Removed)
On 2012/09/19 16:22:43, niemeyer wrote:
> The sorting here doesn't seem to make sense. Besides being wrong (unit/10
comes
> before unit/2), the merge below spoils it even if it's correct.

This is actually the last point at which we can sort, so *were* the sorting not
crack-addled, this would actually be the right place for it. But: good point,
done.

[fwereade, 2012-09-19 17:46:08]

Please take a look.

[niemeyer, 2012-09-20 11:15:22]

LGTM

https://codereview.appspot.com/6533052/diff/8002/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6533052/diff/8002/state/open.go#newcode87
state/open.go:87: relationScopes: db.C("relation-sccopes"),
No dashes please. This would make it very inconvenient to use it from the mongo
shell.

Also, s/cc/c/ :-)

https://codereview.appspot.com/6533052/diff/8002/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6533052/diff/8002/state/relation.go#newcode224
state/relation.go:224: // observe changes to its presence and settings.
We've created the concept of a scope. It won't help if we use the term just in
the method name and continue trying to explain things avoiding the concept of a
scope.

// EnterScope ensures that the unit has entered its scope in the relation.
// A unit is a member of a relation when it has both entered its respective
// scope and its pinger is signaling presence in the environment.

https://codereview.appspot.com/6533052/diff/8002/state/relation.go#newcode249
state/relation.go:249: // relation.
// LeaveScope ensures that the unit has departed its scope in the relation.

https://codereview.appspot.com/6533052/diff/8002/state/relation.go#newcode263
state/relation.go:263: // WatchScope returns a watcher which notifies of
counterpart units
Beautiful. It all makes sense now.

https://codereview.appspot.com/6533052/diff/8002/state/relation.go#newcode322
state/relation.go:322: // relationScopeDoc represents the potential presence of
a unit in a relation.
// relationScopeDoc represents the paticipation of a unit in a relation scope.
// The relation ...

[fwereade, 2012-09-20 11:26:22]

*** Submitted:

state: introduce relation Scope concept

RelationScopeWatcher will be used by RelationUnitsWatcher to determine what
presence keys should be watched for join/depart (and what settings keys
should then be watched when joined).

R=aram, niemeyer
CC=
https://codereview.appspot.com/6533052

https://codereview.appspot.com/6533052/diff/8002/state/open.go
File state/open.go (right):

https://codereview.appspot.com/6533052/diff/8002/state/open.go#newcode87
state/open.go:87: relationScopes: db.C("relation-sccopes"),
On 2012/09/20 11:15:22, niemeyer wrote:
> No dashes please. This would make it very inconvenient to use it from the
mongo
> shell.
> 
> Also, s/cc/c/ :-)

Done.

https://codereview.appspot.com/6533052/diff/8002/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6533052/diff/8002/state/relation.go#newcode224
state/relation.go:224: // observe changes to its presence and settings.
On 2012/09/20 11:15:22, niemeyer wrote:
> We've created the concept of a scope. It won't help if we use the term just in
> the method name and continue trying to explain things avoiding the concept of
a
> scope.
> 
> // EnterScope ensures that the unit has entered its scope in the relation.
> // A unit is a member of a relation when it has both entered its respective
> // scope and its pinger is signaling presence in the environment.

Done.

https://codereview.appspot.com/6533052/diff/8002/state/relation.go#newcode249
state/relation.go:249: // relation.
On 2012/09/20 11:15:22, niemeyer wrote:
> // LeaveScope ensures that the unit has departed its scope in the relation.

Done.

https://codereview.appspot.com/6533052/diff/8002/state/relation.go#newcode263
state/relation.go:263: // WatchScope returns a watcher which notifies of
counterpart units
On 2012/09/20 11:15:22, niemeyer wrote:
> Beautiful. It all makes sense now.
:D

https://codereview.appspot.com/6533052/diff/8002/state/relation.go#newcode322
state/relation.go:322: // relationScopeDoc represents the potential presence of
a unit in a relation.
On 2012/09/20 11:15:22, niemeyer wrote:
> // relationScopeDoc represents the paticipation of a unit in a relation scope.
> // The relation ...

Done.