made hook.Info.Members optional

made hook.Info.Members optional

This allows us to skip saving them, which in turn saves us potentially a lot
of time when trying to store data about 100k-scale relations.

https://code.launchpad.net/~fwereade/juju-core/loose-hook-info-members/+merge/122792

(do not edit description out of merge proposal)

[fwereade, 2012-09-05 05:55:09]

Please take a look.

[TheMue, 2012-09-05 11:47:37]

LGTM, only one little note.

https://codereview.appspot.com/6501086/diff/1/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/6501086/diff/1/worker/uniter/relationer.go#new...
worker/uniter/relationer.go:115: }
Would like a 

switch {
case hi.Kind == hook.RelationDeparted:
    r.ctx.DeleteMember(hi.RemoteUnit)
case hi.RemoteUnit != "":
    r.ctx.UpdateMembers(map[string]map[string]interface{}{hi.RemoteUnit: nil})
}

more.

[niemeyer, 2012-09-05 22:07:32]

The change looks nice on itself, although the high-level plan isn't clear if we
lose that functionality, so a question follows:

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go
File worker/uniter/hook/hook.go (right):

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:68: RelationId int `yaml:"relation-id, omitempty"`
Conventionally, the options aren't spaced after the comma in the field tag. Same
below.

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:78: // Members may contain member unit relation
settings, keyed on unit name.
Slightly hard to get the four nouns in a row. Suggestion:

// Members contains settings for units that are members of the relation,
// keyed on unit name.

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:80: // if a unit is not present, no inferences about
its state can be drawn.
I love the idea of losing the logic and need to save members, but I'm not sure
about what's the plan regarding reporting membership information to hooks. It's
my understanding that we've been diligent about only reporting as present in the
relation the units we've reported to have joined. Are we losing that?

[fwereade, 2012-09-06 07:08:16]

Please take a look.

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go
File worker/uniter/hook/hook.go (right):

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:68: RelationId int `yaml:"relation-id, omitempty"`
On 2012/09/05 22:07:32, niemeyer wrote:
> Conventionally, the options aren't spaced after the comma in the field tag.
Same
> below.

Whoops, thanks

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:78: // Members may contain member unit relation
settings, keyed on unit name.
On 2012/09/05 22:07:32, niemeyer wrote:
> Slightly hard to get the four nouns in a row. Suggestion:
> 
> // Members contains settings for units that are members of the relation,
> // keyed on unit name.

Done.

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:80: // if a unit is not present, no inferences about
its state can be drawn.
On 2012/09/05 22:07:32, niemeyer wrote:
> I love the idea of losing the logic and need to save members, but I'm not sure
> about what's the plan regarding reporting membership information to hooks.
It's
> my understanding that we've been diligent about only reporting as present in
the
> relation the units we've reported to have joined. Are we losing that?

Everything still works: we have enough state in the relation context already,
because *that* is loaded from the persistent relation state; and, hook by hook,
the Relationer can trivially update the context's membership using nothing but
the hook kind and the remote unit. Was very happy when I noticed that we replace
all that stuff with one extra branch in Relationer.PrepareHook :).

https://codereview.appspot.com/6501086/diff/1/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/6501086/diff/1/worker/uniter/relationer.go#new...
worker/uniter/relationer.go:115: }
On 2012/09/05 11:47:37, TheMue wrote:
> Would like a 
> 
> switch {
> case hi.Kind == hook.RelationDeparted:
>     r.ctx.DeleteMember(hi.RemoteUnit)
> case hi.RemoteUnit != "":
>     r.ctx.UpdateMembers(map[string]map[string]interface{}{hi.RemoteUnit: nil})
> }
> 
> more.

I have a bias towards switching on just one thing,but that may just be an
artifact of what I'm used to, I'll give it a go.

[niemeyer, 2012-09-06 13:54:13]

LGTM

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go
File worker/uniter/hook/hook.go (right):

https://codereview.appspot.com/6501086/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:80: // if a unit is not present, no inferences about
its state can be drawn.
On 2012/09/06 07:08:16, fwereade wrote:
> Everything still works: we have enough state in the relation context already,

WOOHAY, then. :-)

https://codereview.appspot.com/6501086/diff/1/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/6501086/diff/1/worker/uniter/relationer.go#new...
worker/uniter/relationer.go:115: }
On 2012/09/05 11:47:37, TheMue wrote:
> Would like a 
> 
> switch {
> case hi.Kind == hook.RelationDeparted:
>     r.ctx.DeleteMember(hi.RemoteUnit)
> case hi.RemoteUnit != "":
>     r.ctx.UpdateMembers(map[string]map[string]interface{}{hi.RemoteUnit: nil})
> }
> 
> more.

This is nice in many cases, but FWIW this case feels to me like abusing switch
to build a simple if/else.

[fwereade, 2012-09-06 16:04:05]

*** Submitted:

made hook.Info.Members optional

This allows us to skip saving them, which in turn saves us potentially a lot
of time when trying to store data about 100k-scale relations.

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6501086

https://codereview.appspot.com/6501086/diff/1/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/6501086/diff/1/worker/uniter/relationer.go#new...
worker/uniter/relationer.go:115: }
On 2012/09/06 13:54:13, niemeyer wrote:
> On 2012/09/05 11:47:37, TheMue wrote:
> > Would like a 
> > 
> > switch {
> > case hi.Kind == hook.RelationDeparted:
> >     r.ctx.DeleteMember(hi.RemoteUnit)
> > case hi.RemoteUnit != "":
> >     r.ctx.UpdateMembers(map[string]map[string]interface{}{hi.RemoteUnit:
nil})
> > }
> > 
> > more.
> 
> This is nice in many cases, but FWIW this case feels to me like abusing switch
> to build a simple if/else.

Reverted cheerfully.