Simpleworker implemention

Simpleworker implemention



https://code.launchpad.net/~natefinch/juju-core/036-simpleworker/+merge/207725

(do not edit description out of merge proposal)

[natefinch, 2014-02-21 19:11:37]

Please take a look.

[natefinch, 2014-02-21 19:12:43]

On 2014/02/21 19:11:37, nate.finch wrote:
> Please take a look.

Forgot to mention in the message... this is a simplified worker that makes it
easy to turn a small amount of code into a worker without having to create a lot
of boilerplate.  It'll be used in the mongo HA stuff, but committing early so
ian can do it.

[natefinch, 2014-02-21 19:15:44]

On 2014/02/21 19:12:43, nate.finch wrote:
> On 2014/02/21 19:11:37, nate.finch wrote:
> > Please take a look.
> 
> Forgot to mention in the message... this is a simplified worker that makes it
> easy to turn a small amount of code into a worker without having to create a
lot
> of boilerplate.  It'll be used in the mongo HA stuff, but committing early so
> ian can do it.

s/do it/use it/

[rog, 2014-02-24 17:57:35]

LGTM with a couple of suggestions below, thanks.

https://codereview.appspot.com/67080043/diff/1/worker/simpleworker.go
File worker/simpleworker.go (right):

https://codereview.appspot.com/67080043/diff/1/worker/simpleworker.go#newcode7
worker/simpleworker.go:7: // mechanisms through the use of a couple channels. 
The channels are only used
A slightly different suggestion:

// simpleWorker implements the worker returned by NewSimpleWorker.
// The stopc and done channels are used for closing notifications
// only. No values are sent over them. The err value is set once only,
// just before the done channel is closed.

?

https://codereview.appspot.com/67080043/diff/1/worker/simpleworker_test.go
File worker/simpleworker_test.go (right):

https://codereview.appspot.com/67080043/diff/1/worker/simpleworker_test.go#ne...
worker/simpleworker_test.go:39: c.Assert(w.Wait(), gc.Equals, nil)
This isn't quite testing that we're waiting until doWork
returns (perhaps we're not actually seeing the value
returned by doWork at all). If we returned testError from the function,
the test would be more convincing.

Then perhaps one more test to check that the
happy path works.

[natefinch, 2014-02-24 19:26:43]

https://codereview.appspot.com/67080043/diff/1/worker/simpleworker.go
File worker/simpleworker.go (right):

https://codereview.appspot.com/67080043/diff/1/worker/simpleworker.go#newcode7
worker/simpleworker.go:7: // mechanisms through the use of a couple channels. 
The channels are only used
On 2014/02/24 17:57:36, rog wrote:
> A slightly different suggestion:
> 
> // simpleWorker implements the worker returned by NewSimpleWorker.
> // The stopc and done channels are used for closing notifications
> // only. No values are sent over them. The err value is set once only,
> // just before the done channel is closed.
> 
> ?

Sounds good. Done.

https://codereview.appspot.com/67080043/diff/1/worker/simpleworker_test.go
File worker/simpleworker_test.go (right):

https://codereview.appspot.com/67080043/diff/1/worker/simpleworker_test.go#ne...
worker/simpleworker_test.go:39: c.Assert(w.Wait(), gc.Equals, nil)
On 2014/02/24 17:57:36, rog wrote:
> This isn't quite testing that we're waiting until doWork
> returns (perhaps we're not actually seeing the value
> returned by doWork at all). If we returned testError from the function,
> the test would be more convincing.

Good point. Done.

> Then perhaps one more test to check that the
> happy path works.


Added a test that nil gets passed through Wait().