Add unitScopePath (internal to state)

Add unitScopePath (internal to state)

This is a very simple type that just wraps some common ZK path operations
necessary for managing unit relations; it won't be used *widely*, but it
should make a few forthcoming code paths a little clearer (and avoid the
python situation in which scope-related code is scattered across several
types).

https://code.launchpad.net/~fwereade/juju-core/unit-scope/+merge/110839

(do not edit description out of merge proposal)

[fwereade, 2012-06-18 14:49:42]

Please take a look.

[niemeyer, 2012-06-19 20:39:36]

Idea looks good. Some suggestions:

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

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode95
state/relation.go:95: // relation that is located within a particular container.
This is a precise description of what the relation scope is.

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode99
state/relation.go:99: //   /relations/<relation-id>/<container-id>
And this is also the path for the relation scope.

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode100
state/relation.go:100: type unitScope struct {
type relationScopePath string?

See below regarding zk.

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode108
state/relation.go:108: func (s *unitScope) SettingsPath(key string) string {
s/SettingsPath/settingsPath/

The type and its methods are all internal, so we can have them lowercased to
make it more clear. I suggested breaking this pattern before in topology because
the type is so extensive and has lots of helpers, but it doesn't seem worth it
here.

Same below for other methods.

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode115
state/relation.go:115: func (s *unitScope) PresencePath(role RelationRole, key
string) string {
s/key/unitKey

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode121
state/relation.go:121: func (s *unitScope) PrepareJoin(role RelationRole) error
{
// create creates the relation scope path and any necessary subpaths.
func (p relationScopePath) create(zk *zookeeper.Conn, roles []RelationRole)
error

The set of roles means we can create all the right nodes at once, without having
to skip bad retries in follow up calls.

[fwereade, 2012-06-19 22:41:27]

Please take a look.

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

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode95
state/relation.go:95: // relation that is located within a particular container.
On 2012/06/19 20:39:36, niemeyer wrote:
> This is a precise description of what the relation scope is.

...except that the term RelationScope is used to indicate what possible scope
path*s* could be in play in a particular relation. Saying, or implying, that a
relation has one relation scope but multiple relation scope paths strikes me as
somewhat confusing.

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode99
state/relation.go:99: //   /relations/<relation-id>/<container-id>
On 2012/06/19 20:39:36, niemeyer wrote:
> And this is also the path for the relation scope.

Or rather, *one* of the "relation scope"s, which have a somewhat subtle
relationship with the relation's actual RelationScope (which is singular).

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode100
state/relation.go:100: type unitScope struct {
On 2012/06/19 20:39:36, niemeyer wrote:
> type relationScopePath string?
> 
> See below regarding zk.

I think this may be subtly misleading: a relation has a RelationScope, which
does not necessarily correspond to a particular path; but a given *unit* in a
relation always corresponds to exactly one of the (potentially many) scope paths
relevant to the relation. Honestly, I'd be happier with "type unitScopePath
string".

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode108
state/relation.go:108: func (s *unitScope) SettingsPath(key string) string {
On 2012/06/19 20:39:36, niemeyer wrote:
> s/SettingsPath/settingsPath/
> 
> The type and its methods are all internal, so we can have them lowercased to
> make it more clear. I suggested breaking this pattern before in topology
because
> the type is so extensive and has lots of helpers, but it doesn't seem worth it
> here.
> 
> Same below for other methods.

Intent was to distinguish between subpath, which clients don;t really have any
business calling directly, and the other methods which they do. But, perfectly
sensible, done.

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode115
state/relation.go:115: func (s *unitScope) PresencePath(role RelationRole, key
string) string {
On 2012/06/19 20:39:36, niemeyer wrote:
> s/key/unitKey

Done.

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode121
state/relation.go:121: func (s *unitScope) PrepareJoin(role RelationRole) error
{
On 2012/06/19 20:39:36, niemeyer wrote:
> // create creates the relation scope path and any necessary subpaths.
> func (p relationScopePath) create(zk *zookeeper.Conn, roles []RelationRole)
> error

Much nicer to pass the zk here, thanks. Done.

> The set of roles means we can create all the right nodes at once, without
having
> to skip bad retries in follow up calls.

But I'm not sure about this. The intent of this method is that a unit agent call
it prior to joining, to ensure that the parents of the nodes it intends to write
exist already.

I don't think it's sensible to assume otherwise; and I don't see a cheaper way
of ensuring the nodes' existence than just trying to create them: that's just
one round-trip per node. I don't think it's sane or safe to infer (say) the
settings node's existence from the fact of any role node's existence, so one way
or another we have to check, and I don't see why it would be better to have up
to 2 round-trips per node when we can get everything we need with just one.

Furthermore, making a unit agent directly responsible for creating not only the
nodes it uses but the nodes that other units might want to use just means we're
potentially checking/hitting more nodes every time we're about to join.

Are you sure about this?

[niemeyer, 2012-06-20 00:17:52]

LGTM

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

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode95
state/relation.go:95: // relation that is located within a particular container.
On 2012/06/19 22:41:27, fwereade wrote:
> On 2012/06/19 20:39:36, niemeyer wrote:
> > This is a precise description of what the relation scope is.
> 
> ...except that the term RelationScope is used to indicate what possible scope
> path*s* could be in play in a particular relation. Saying, or implying, that a
> relation has one relation scope but multiple relation scope paths strikes me
as
> somewhat confusing.

This is a minor implementation detail that will remain contained here and is
probably not worth our time arguing, so I'm happy with what you decide.

Just keep in mind that you're introducing an overlapping term ("unit scope")
that is unnecessary. We have a very clear idea of what the relation scope is,
both internally and in external documentation, and it matches your description
of what a unit scope is exactly.

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode99
state/relation.go:99: //   /relations/<relation-id>/<container-id>
On 2012/06/19 22:41:27, fwereade wrote:
> On 2012/06/19 20:39:36, niemeyer wrote:
> > And this is also the path for the relation scope.
> 
> Or rather, *one* of the "relation scope"s, which have a somewhat subtle
> relationship with the relation's actual RelationScope (which is singular).

In my head RelationScope is a flag pointing out which scoping strategy we should
use. If it is set to ScopeContainer, we have multiple relation scopes per
relation (one scope per container).

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode121
state/relation.go:121: func (s *unitScope) PrepareJoin(role RelationRole) error
{
On 2012/06/19 22:41:27, fwereade wrote:
> On 2012/06/19 20:39:36, niemeyer wrote:
> > // create creates the relation scope path and any necessary subpaths.
> > func (p relationScopePath) create(zk *zookeeper.Conn, roles []RelationRole)
> > error
> 
> Much nicer to pass the zk here, thanks. Done.
> 
> > The set of roles means we can create all the right nodes at once, without
> having
> > to skip bad retries in follow up calls.
> 
> But I'm not sure about this. The intent of this method is that a unit agent
call
> it prior to joining, to ensure that the parents of the nodes it intends to
write
> exist already.
> 
> I don't think it's sensible to assume otherwise; and I don't see a cheaper way
> of ensuring the nodes' existence than just trying to create them: that's just
> one round-trip per node. I don't think it's sane or safe to infer (say) the
> settings node's existence from the fact of any role node's existence, so one
way
> or another we have to check, and I don't see why it would be better to have up
> to 2 round-trips per node when we can get everything we need with just one.
> 
> Furthermore, making a unit agent directly responsible for creating not only
the
> nodes it uses but the nodes that other units might want to use just means
we're
> potentially checking/hitting more nodes every time we're about to join.
> 
> Are you sure about this?

I'm not. You clearly have more context than I do. I'm happy with moving your
way.

[fwereade, 2012-06-20 06:10:36]

*** Submitted:

Add unitScopePath (internal to state)

This is a very simple type that just wraps some common ZK path operations
necessary for managing unit relations; it won't be used *widely*, but it
should make a few forthcoming code paths a little clearer (and avoid the
python situation in which scope-related code is scattered across several
types).

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

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

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode95
state/relation.go:95: // relation that is located within a particular container.
On 2012/06/20 00:17:52, niemeyer wrote:
> On 2012/06/19 22:41:27, fwereade wrote:
> > On 2012/06/19 20:39:36, niemeyer wrote:
> > > This is a precise description of what the relation scope is.
> > 
> > ...except that the term RelationScope is used to indicate what possible
scope
> > path*s* could be in play in a particular relation. Saying, or implying, that
a
> > relation has one relation scope but multiple relation scope paths strikes me
> as
> > somewhat confusing.
> 
> This is a minor implementation detail that will remain contained here and is
> probably not worth our time arguing, so I'm happy with what you decide.
> 
> Just keep in mind that you're introducing an overlapping term ("unit scope")
> that is unnecessary. We have a very clear idea of what the relation scope is,
> both internally and in external documentation, and it matches your description
> of what a unit scope is exactly.

Heh, quite possibly you'll see an ignominious s/unitScopePath/relationScopePath/
CL at some stage soon, when I see the light; but for now, thanks :).

https://codereview.appspot.com/6305107/diff/1/state/relation.go#newcode121
state/relation.go:121: func (s *unitScope) PrepareJoin(role RelationRole) error
{
On 2012/06/20 00:17:52, niemeyer wrote:
> On 2012/06/19 22:41:27, fwereade wrote:
> > On 2012/06/19 20:39:36, niemeyer wrote:
> > > // create creates the relation scope path and any necessary subpaths.
> > > func (p relationScopePath) create(zk *zookeeper.Conn, roles
[]RelationRole)
> > > error
> > 
> > Much nicer to pass the zk here, thanks. Done.
> > 
> > > The set of roles means we can create all the right nodes at once, without
> > having
> > > to skip bad retries in follow up calls.
> > 
> > But I'm not sure about this. The intent of this method is that a unit agent
> call
> > it prior to joining, to ensure that the parents of the nodes it intends to
> write
> > exist already.
> > 
> > I don't think it's sensible to assume otherwise; and I don't see a cheaper
way
> > of ensuring the nodes' existence than just trying to create them: that's
just
> > one round-trip per node. I don't think it's sane or safe to infer (say) the
> > settings node's existence from the fact of any role node's existence, so one
> way
> > or another we have to check, and I don't see why it would be better to have
up
> > to 2 round-trips per node when we can get everything we need with just one.
> > 
> > Furthermore, making a unit agent directly responsible for creating not only
> the
> > nodes it uses but the nodes that other units might want to use just means
> we're
> > potentially checking/hitting more nodes every time we're about to join.
> > 
> > Are you sure about this?
> 
> I'm not. You clearly have more context than I do. I'm happy with moving your
> way.

Cheers :).