add RelationContext type

add RelationContext type

The RelationContext manages a single relation's unit settings, as a step
towards implementing relation-aware jujuc commands. It exposes features that
will be used directly by the relation-get, relation-set, relation-list and
relation-ids commands; the interface is designed to allow convenient
creation from uniter.RelationState instances, and convenient updates from
uniter.HookInfo values.

This doesn't yet fit cleanly with the ClientContext type, but integrating
the types will have somewhat noisy consequences, so the RelationContext is
currently exercised alone. Expect an imminent followup integrating
RelationContext into ClientContext as follows:

  drop LocalUnitName string; add Unit *state.Unit
  drop RelationName string; add RelationId int
  keep RemoteUnitName
  add Relations map[int]*RelationContext, keyed on relation id

...and:

  change name to HookContext

We expect every HookContext to contain full details of all relations the
unit is currently participating in, in Relations; this is needed to allow
commands like relation-ids and relation-list to run regardless of context.
The relation-specific JUJU_RELATION and JUJU_RELATION_ID environment
variables (along with JUJU_REMOTE_UNIT where apropriate) would be set when
RelationId were valid (ie it must not be -1, and it must bea valid key into
Relations).

https://code.launchpad.net/~fwereade/juju-core/jujuc-server-relation-context/+merge/117202

(do not edit description out of merge proposal)

[fwereade, 2012-07-30 01:05:38]

Please take a look.

[rog, 2012-07-30 09:11:41]

looking nice. minor comments below.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:104: st *state.State
This field seems unused currently. If it went, we could lose the State argument
to NewRelationContext? Any particular reason we need it?

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:115: units []string
why maintain this independently?
we could make RelationContext.Units manufacture it on demand; then we wouldn't
have to worry about consistency with members.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:125: // determined by the keys in members.
this reads a little awkwardly to me.
how about something like:
// NewRelationContext creates a new context for the given
// relation unit. The unit-name keys of members supplies the
// initial membership.

out of interest, why not map[string]bool?

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:154: ctx.units = nil
ctx.units = ctxt.units[:0]
and save some allocations for free.
or leave it out, and make the slice when Units is called, as suggested above.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:174: // Units returns the names of the units that
are present in the relation.
// in alphabetical order.
?

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:176: return ctx.units
manufacture on demand?

[fwereade, 2012-07-30 09:21:00]

Cheers.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:115: units []string
On 2012/07/30 09:11:41, rog wrote:
> why maintain this independently?
> we could make RelationContext.Units manufacture it on demand; then we wouldn't
> have to worry about consistency with members.

I don't want to keep doing building it every time someone calls relation-list
(consider 10,000 units case), so I want to cache it somehow; if I were to
somehow mark it dirty for lazy recalculation (not just nil, nil is valid), I'd
need to do so in Update; feels like I may as well just do it all there are get
away with simpler fields. Update will be called once per hook for each relation,
which doesn't feel like an overwhelming amount of extra work; and it keeps the
relevant code in just one place.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:125: // determined by the keys in members.
On 2012/07/30 09:11:41, rog wrote:
> this reads a little awkwardly to me.
> how about something like:
> // NewRelationContext creates a new context for the given
> // relation unit. The unit-name keys of members supplies the
> // initial membership.

Yeah, something like that, cheers.

> out of interest, why not map[string]bool?

Purely for the convenience of loading from uniter.RelationState, which has a
map[string]int membership map containing settings versions which we don;t really
care about here.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:154: ctx.units = nil
On 2012/07/30 09:11:41, rog wrote:
> ctx.units = ctxt.units[:0]
> and save some allocations for free.
> or leave it out, and make the slice when Units is called, as suggested above.

Nice, thanks

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:174: // Units returns the names of the units that
are present in the relation.
On 2012/07/30 09:11:41, rog wrote:
> // in alphabetical order.
> ?

Good point.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:176: return ctx.units
On 2012/07/30 09:11:41, rog wrote:
> manufacture on demand?

Not 100% sure justified, we'll talk live.

[fwereade, 2012-07-30 13:55:17]

Please take a look.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:104: st *state.State
On 2012/07/30 09:11:41, rog wrote:
> This field seems unused currently. If it went, we could lose the State
argument
> to NewRelationContext? Any particular reason we need it?

Woohoo! Well spotted :). Done.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:115: units []string
On 2012/07/30 09:21:00, fwereade wrote:
> On 2012/07/30 09:11:41, rog wrote:
> > why maintain this independently?
> > we could make RelationContext.Units manufacture it on demand; then we
wouldn't
> > have to worry about consistency with members.
> 
> I don't want to keep doing building it every time someone calls relation-list
> (consider 10,000 units case), so I want to cache it somehow; if I were to
> somehow mark it dirty for lazy recalculation (not just nil, nil is valid), I'd
> need to do so in Update; feels like I may as well just do it all there are get
> away with simpler fields. Update will be called once per hook for each
relation,
> which doesn't feel like an overwhelming amount of extra work; and it keeps the
> relevant code in just one place.

Discussed live, on-demand is fine

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:125: // determined by the keys in members.
On 2012/07/30 09:21:00, fwereade wrote:
> On 2012/07/30 09:11:41, rog wrote:
> > this reads a little awkwardly to me.
> > how about something like:
> > // NewRelationContext creates a new context for the given
> > // relation unit. The unit-name keys of members supplies the
> > // initial membership.
> 
> Yeah, something like that, cheers.
> 
> > out of interest, why not map[string]bool?
> 
> Purely for the convenience of loading from uniter.RelationState, which has a
> map[string]int membership map containing settings versions which we don;t
really
> care about here.

Done.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:154: ctx.units = nil
On 2012/07/30 09:21:00, fwereade wrote:
> On 2012/07/30 09:11:41, rog wrote:
> > ctx.units = ctxt.units[:0]
> > and save some allocations for free.
> > or leave it out, and make the slice when Units is called, as suggested
above.
> 
> Nice, thanks

Not actually done, because of on-demand Units

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:174: // Units returns the names of the units that
are present in the relation.
On 2012/07/30 09:21:00, fwereade wrote:
> On 2012/07/30 09:11:41, rog wrote:
> > // in alphabetical order.
> > ?
> 
> Good point.

Done.

https://codereview.appspot.com/6448074/diff/2001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:176: return ctx.units
On 2012/07/30 09:21:00, fwereade wrote:
> On 2012/07/30 09:11:41, rog wrote:
> > manufacture on demand?
> 
> Not 100% sure justified, we'll talk live.

Done.

[niemeyer, 2012-07-31 19:56:45]

I seem to be missing some of the behavior there, but otherwise find it
surprisingly fresh.

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:114: units []string
This is unused.

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:131: ctx.Flush(false)
Error checking?

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:135: // Flush clears the cached data for non-member
units, making the context
In a context of delivering a cached delta, "flushing" doesn't generally clear
stuff up. I suggest calling this Reset or similar.

I'm also confused by the logic within the function. If you ask it not to write,
it resets the map. If you ask it to write, it keeps cached data in memory and
doesn't actually clear? Either I'm missing something or the API is not entirely
sound. Maybe this should be split into more specific methods.

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:151: ctx.members = members
s/Update/SetMembers/?

Also, settingsMap should be public if we're using it as an argument of a method
that is public.

[fwereade, 2012-07-31 20:57:57]

Please take a look.

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:114: units []string
On 2012/07/31 19:56:45, niemeyer wrote:
> This is unused.

Done.

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:131: ctx.Flush(false)
On 2012/07/31 19:56:45, niemeyer wrote:
> Error checking?

Can't fail, as it happens, but I know it is evil to assume such things :).

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:135: // Flush clears the cached data for non-member
units, making the context
On 2012/07/31 19:56:45, niemeyer wrote:
> In a context of delivering a cached delta, "flushing" doesn't generally clear
> stuff up. I suggest calling this Reset or similar.
> 
> I'm also confused by the logic within the function. If you ask it not to
write,
> it resets the map. If you ask it to write, it keeps cached data in memory and
> doesn't actually clear? Either I'm missing something or the API is not
entirely
> sound. Maybe this should be split into more specific methods.

WriteSettings/ClearCache SGTM. Sorry crackful logic.

https://codereview.appspot.com/6448074/diff/6001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:151: ctx.members = members
On 2012/07/31 19:56:45, niemeyer wrote:
> s/Update/SetMembers/?
> 
> Also, settingsMap should be public if we're using it as an argument of a
method
> that is public.

Done.

[niemeyer, 2012-07-31 21:07:23]

LGTM.. only doc suggestions:

https://codereview.appspot.com/6448074/diff/9001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6448074/diff/9001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:130: // WriteSettings persists all changes made to
Settings
s/Settings/the unit's relation settings./

https://codereview.appspot.com/6448074/diff/9001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:138: // ClearCache discards all cached non-member
unit settings, and Settings,
// ClearCache discards all cached settings, both for the unit and
// for the remote members.

[fwereade, 2012-07-31 21:45:40]

*** Submitted:

add RelationContext type

The RelationContext manages a single relation's unit settings, as a step
towards implementing relation-aware jujuc commands. It exposes features that
will be used directly by the relation-get, relation-set, relation-list and
relation-ids commands; the interface is designed to allow convenient
creation from uniter.RelationState instances, and convenient updates from
uniter.HookInfo values.

This doesn't yet fit cleanly with the ClientContext type, but integrating
the types will have somewhat noisy consequences, so the RelationContext is
currently exercised alone. Expect an imminent followup integrating
RelationContext into ClientContext as follows:

  drop LocalUnitName string; add Unit *state.Unit
  drop RelationName string; add RelationId int
  keep RemoteUnitName
  add Relations map[int]*RelationContext, keyed on relation id

...and:

  change name to HookContext

We expect every HookContext to contain full details of all relations the
unit is currently participating in, in Relations; this is needed to allow
commands like relation-ids and relation-list to run regardless of context.
The relation-specific JUJU_RELATION and JUJU_RELATION_ID environment
variables (along with JUJU_REMOTE_UNIT where apropriate) would be set when
RelationId were valid (ie it must not be -1, and it must bea valid key into
Relations).

R=rog, niemeyer
CC=
https://codereview.appspot.com/6448074

https://codereview.appspot.com/6448074/diff/9001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/6448074/diff/9001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:130: // WriteSettings persists all changes made to
Settings
On 2012/07/31 21:07:23, niemeyer wrote:
> s/Settings/the unit's relation settings./

Done.

https://codereview.appspot.com/6448074/diff/9001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:138: // ClearCache discards all cached non-member
unit settings, and Settings,
On 2012/07/31 21:07:23, niemeyer wrote:
> // ClearCache discards all cached settings, both for the unit and
> // for the remote members.

Done.