Merge lp://staging/~fwereade/juju-core/cleaner-resumer-integrate into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+174013_code.launchpad.net,

Message:
Please take a look.

Description:
jujud: integrate cleaner, resumer

the stateReporter chan may be useful elsewhere, but I haven't trawled
for
further simplifications. Main point is integrating these tasks and
testing wat
we can.

https://code.launchpad.net/~fwereade/juju-core/cleaner-resumer-integrate/+merge/174013

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10825044/

Affected files:
   A [revision details]
   M cmd/jujud/machine.go
   M cmd/jujud/machine_test.go

LGTM, only a couple of clarification requests.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode318
cmd/jujud/machine_test.go:318: agentStates := make(chan *state.State,
1000)
why 1000? why not unbuffered?

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode319
cmd/jujud/machine_test.go:319: defer
sendOpenedStates(sendOpenedStates(agentStates))
this a but confusing - perhaps a comment before this whole block
explaining what's being done will help

https://codereview.appspot.com/10825044/

LGTM

I have some comments, but I can live with the code as it is written. So
if having a helper is too much distraction, just land it.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode318
cmd/jujud/machine_test.go:318: agentStates := make(chan *state.State,
1000)
On 2013/07/10 18:28:46, dimitern wrote:
> why 1000? why not unbuffered?

I don't think we want unbuffered, because then when the other loop
starts up it will block until we do our select (I think).
The trick is that once you call 'sendOpenedStates' *all* things that
start up are going to be trying to send their state to this channel.

The general idea is: "I'm going to start something up, and I want a
backdoor into the State object it is using, so send it onto this channel
so I can pull it out in this code."

However, once you set the channel, more things are going to be sent on
it than you really need. We could just create a local goroutine that
consumes them all and puts it somewhere we can use it.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode319
cmd/jujud/machine_test.go:319: defer
sendOpenedStates(sendOpenedStates(agentStates))
On 2013/07/10 18:28:46, dimitern wrote:
> this a but confusing - perhaps a comment before this whole block
explaining
> what's being done will help

A slightly better pattern is probably:

oldChan := sendOpenedStates(agentStates)
defer sendOpenedStates(oldChan)

For things like this, I usually prefer to have the sendOpenedStates type
function actually return a func() that does the cleanup. So the code
becomes:

func sendOpenedStates(ch <-chan *state.State) {
   old, agentStates := agentStates, ch
   return func() { agentStates := old) }
}

As then the code in the test case is:

cleanup := sendOpenedStates(agentStates)
defer cleanup()

And that is a common pattern regardless of what internal poking a given
test helper has to do.

Nested function calls and defer are hard to parse for mere mortals,
which is why I don't spell it as:

defer sendOpenedStates(agentStates)()
     ^------------------------------^
The indicator of what you are deferring is very far from 'defer' vs

cleanup := ...
defer cleanup()

It is also why I'm not 100% sold on the:

go func() {
...
...
...
...
...
..
...
.
.
.
.
..
}()

Syntax. At least it is "go func() {" which makes it clear you are
thinking about calling func at the end of this. But when you leave off
those last two parenthesis it gets pretty confusing. :)

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode329
cmd/jujud/machine_test.go:329: case <-time.After(500 *
time.Millisecond):
Please use the testing.LongWait constants. It will help signal *why* you
are picking a given timeout, and lets us tweak them across the codebase
if we find they are too long/short.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode386
cmd/jujud/machine_test.go:386: timeout := time.After(500 *
time.Millisecond)
Same here on the timeout.

This loop has been used elsewhere, but I'm not quite sure why we hav...

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode318
cmd/jujud/machine_test.go:318: agentStates := make(chan *state.State,
1000)
On 2013/07/11 05:50:16, jameinel wrote:
> On 2013/07/10 18:28:46, dimitern wrote:
> > why 1000? why not unbuffered?

> I don't think we want unbuffered, because then when the other loop
starts up it
> will block until we do our select (I think).
> The trick is that once you call 'sendOpenedStates' *all* things that
start up
> are going to be trying to send their state to this channel.

> The general idea is: "I'm going to start something up, and I want a
backdoor
> into the State object it is using, so send it onto this channel so I
can pull it
> out in this code."

> However, once you set the channel, more things are going to be sent on
it than
> you really need. We could just create a local goroutine that consumes
them all
> and puts it somewhere we can use it.

In practice, only one state should be sent, because we're not expecting
errors and restarts. But if we get them, that buffer should be big
enough to ensure nothing blocks. That's all :). I don't *think* it's
worth the extra infrastructure to check that no more come in; if things
are broken I think we'll detect it in other ways.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode319
cmd/jujud/machine_test.go:319: defer
sendOpenedStates(sendOpenedStates(agentStates))
On 2013/07/11 05:50:16, jameinel wrote:
> On 2013/07/10 18:28:46, dimitern wrote:
> > this a but confusing - perhaps a comment before this whole block
explaining
> > what's being done will help

> A slightly better pattern is probably:

> oldChan := sendOpenedStates(agentStates)
> defer sendOpenedStates(oldChan)

> For things like this, I usually prefer to have the sendOpenedStates
type
> function actually return a func() that does the cleanup. So the code
becomes:

> func sendOpenedStates(ch <-chan *state.State) {
> old, agentStates := agentStates, ch
> return func() { agentStates := old) }
> }

> As then the code in the test case is:

> cleanup := sendOpenedStates(agentStates)
> defer cleanup()

> And that is a common pattern regardless of what internal poking a
given test
> helper has to do.

> Nested function calls and defer are hard to parse for mere mortals,
which is why
> I don't spell it as:

> defer sendOpenedStates(agentStates)()
> ^------------------------------^
> The indicator of what you are deferring is very far from 'defer' vs

> cleanup := ...
> defer cleanup()

> It is also why I'm not 100% sold on the:

> go func() {
> ...
> ...
> ...
> ...
> ...
> ..
> ...
> .
> .
> .
> .
> ..
> }()

> Syntax. At least it is "go func() {" which makes it clear you are
thinking about
> calling func at the end of this. But when you leave off those last two
> parenthesis it gets pretty confusing. :)

Done as undo func.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode329
cmd/jujud/machine_test.go:329: case <-time.After(500 *
time.Millisecond):
On 2013/07/11 05:50:16, jameinel wrot...