Add parallel runner utility.

Add parallel runner utility.

Following Roger's advice, this branch adds a new parallel runner utility in utils/parallel. The utility is used to replace the custom code in the Azure provider which deletes the machines concurrently.  The utility is taken from Roger's https://code.google.com/p/rog-go/ and slightly adapted (couple of variable renamed, usage of the testing utilities from juju).  The code from the provider is unit-tested but since the new code does exactly the same as the old one, the tests are not affected.

https://code.launchpad.net/~rvb/juju-core/parallel-helper/+merge/179941

(do not edit description out of merge proposal)

[rvb, 2013-08-13 15:13:02]

Please take a look.

[rog, 2013-08-13 15:16:31]

On 2013/08/13 15:13:02, rvb wrote:
> Please take a look.

LGTM

[jameinel, 2013-08-14 10:19:13]

I have some comments about the structure, but nothing that needs to block.

LGTM

https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go
File utils/parallel/parallel.go (right):

https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go#new...
utils/parallel/parallel.go:37: func NewRun(maxPar int) *Run {
It would be nice if you could just rename this to "maxParallel". Or even just
"num".

https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go#new...
utils/parallel/parallel.go:53: parallelRun.err <- errs
There is a fair bit of complexity to make a slice of errors look like a single
error. Why not just define the 'err' channel to take an Errors object? rather
than 'chan error' ?

https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go#new...
utils/parallel/parallel.go:70: <-parallelRun.limiter
Wasn't there a bug about pushing into a buffered channel and pulling out of it
not properly synchronizing? (At least the race detector would complain about
it.)

I thought the 'correct' way was on init you create the buffered, fill it, and
then pull out and push back for synchronization.

Maybe it doesn't matter as this is "don't start yet" rather than memory
synchcronization.

[rog, 2013-08-14 10:47:20]

https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go
File utils/parallel/parallel.go (right):

https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go#new...
utils/parallel/parallel.go:51: // TODO sort errors by original order of Do
request?
s/TODO/TODO(rog)/

https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go#new...
utils/parallel/parallel.go:53: parallelRun.err <- errs
On 2013/08/14 10:19:13, jameinel wrote:
> There is a fair bit of complexity to make a slice of errors look like a single
> error. Why not just define the 'err' channel to take an Errors object? rather
> than 'chan error' ?

The reason for that is that we want Wait to return error rather than Errors and
we don't want it to return a non-nil error when there are no errors. We could
put the "if len(errs) > 0" check inside Wait instead, but it just moves code
around - I don't think it saves anything (it also means that Wait can be
agnostic of the underlying type, whereas this function needs to know all about
Errors anyway, so it may as well do the check here).

https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go#new...
utils/parallel/parallel.go:70: <-parallelRun.limiter
On 2013/08/14 10:19:13, jameinel wrote:
> Maybe it doesn't matter as this is "don't start yet" rather than memory
> synchcronization.

This. I was of your opinion, but have come around to Gustavo's point of view on
this after an involved discussion in golang-dev.

The tests pass fine with the race detector BTW.

[rvb, 2013-08-14 12:33:11]

Thanks for the reviews guys!

> **jam**
>
>
https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go#new...
>utils/parallel/parallel.go:37: func NewRun(maxPar int) *Run {
> It would be nice if you could just rename this to "maxParallel". Or even just
> "num".

Done.

> **rog**
>
> https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go
> File utils/parallel/parallel.go (right):
> 
>
https://codereview.appspot.com/12861043/diff/1/utils/parallel/parallel.go#new...
> utils/parallel/parallel.go:51: // TODO sort errors by original order of Do
> request?
> s/TODO/TODO(rog)/

Fixed.

> **rog**
>
> The tests pass fine with the race detector BTW.

Nice!