utils/parallel: concurrent attempts

utils/parallel: concurrent attempts

Introducing a new type, parallel.Try, which should
be a useful primitive when making concurrent connection
attempts.

https://code.launchpad.net/~rogpeppe/juju-core/473-parallel-try/+merge/199099

(do not edit description out of merge proposal)

[natefinch, 2013-12-16 13:32:23]

LGTM.  Ideally I'd like to see this as a standalone package, but I know that has
some overhead.

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

https://codereview.appspot.com/42760043/diff/1/utils/parallel/try.go#newcode133
utils/parallel/try.go:133: // to limit memory usage by waiting goroutines.
this comment is confusing, can you clarify?

https://codereview.appspot.com/42760043/diff/1/utils/parallel/try.go#newcode135
utils/parallel/try.go:135: f := func() {
can we make this a separate function?  we don't seem to gain much by putting it
inline.

https://codereview.appspot.com/42760043/diff/1/utils/parallel/try.go#newcode159
utils/parallel/try.go:159: func (c *Try) Close() {
shouldn't this be a t and not a c?

[rog, 2013-12-16 15:34:19]

Please take a look.

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

https://codereview.appspot.com/42760043/diff/1/utils/parallel/try.go#newcode133
utils/parallel/try.go:133: // to limit memory usage by waiting goroutines.
On 2013/12/16 13:32:25, nate.finch wrote:
> this comment is confusing, can you clarify?

I've changed the implementation instead to do what I was alluding to. It's more
useful behaviour, and it means we don't end up writing code that accidentally
relies on the always-non-blocking nature of Start.

https://codereview.appspot.com/42760043/diff/1/utils/parallel/try.go#newcode135
utils/parallel/try.go:135: f := func() {
On 2013/12/16 13:32:25, nate.finch wrote:
> can we make this a separate function?  we don't seem to gain much by putting
it
> inline.

Well, it's got to be a closure otherwise we can't send it
on the channel. I'm not sure that doing something like

f := func() {
    t.runTry(try)
}

will make things much clearer.

By having it as a closure here, it makes
it obvious that it's not called directly.

https://codereview.appspot.com/42760043/diff/1/utils/parallel/try.go#newcode159
utils/parallel/try.go:159: func (c *Try) Close() {
On 2013/12/16 13:32:25, nate.finch wrote:
> shouldn't this be a t and not a c?

Done (my original name for this type was Connector)

[natefinch, 2013-12-16 16:06:06]

On 2013/12/16 15:34:19, rog wrote:
>
https://codereview.appspot.com/42760043/diff/1/utils/parallel/try.go#newcode135
> utils/parallel/try.go:135: f := func() {
> On 2013/12/16 13:32:25, nate.finch wrote:
> > can we make this a separate function?  we don't seem to gain much by putting
> it
> > inline.
> 
> Well, it's got to be a closure otherwise we can't send it
> on the channel. I'm not sure that doing something like
> 
> f := func() {
>     t.runTry(try)
> }
> 
> will make things much clearer.
> 
> By having it as a closure here, it makes
> it obvious that it's not called directly.

Ahh, yeah, of course. I wasn't thinking about the fact it needs to be a closure.


I like the new changes.

LGTM

[rog, 2013-12-16 16:25:05]

Please take a look.

[dimitern, 2013-12-17 11:06:37]

LGTM, apart from a few typos and imports ordering.

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try.go
File utils/parallel/try.go (right):

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try.go#newc...
utils/parallel/try.go:6: "launchpad.net/tomb"
import (
  "errors"
  "io"
  ...

  "launchpad.net/tomb"
)

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try.go#newc...
utils/parallel/try.go:15: type Try struct {
a doc comment would be nice.

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try.go#newc...
utils/parallel/try.go:30: // positive, it limits the nunber of concurrently
running functions.
s/nunber/number/

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try_test.go
File utils/parallel/try_test.go (right):

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try_test.go...
utils/parallel/try_test.go:10: gc "launchpad.net/gocheck"
please put gocheck on a separate line, after std imports

[rog, 2013-12-17 11:23:49]

Please take a look.

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try.go
File utils/parallel/try.go (right):

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try.go#newc...
utils/parallel/try.go:6: "launchpad.net/tomb"
On 2013/12/17 11:06:37, dimitern wrote:
> import (
>   "errors"
>   "io"
>   ...
> 
>   "launchpad.net/tomb"
> )

Done.

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try.go#newc...
utils/parallel/try.go:15: type Try struct {
On 2013/12/17 11:06:37, dimitern wrote:
> a doc comment would be nice.

Done.

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try.go#newc...
utils/parallel/try.go:30: // positive, it limits the nunber of concurrently
running functions.
On 2013/12/17 11:06:37, dimitern wrote:
> s/nunber/number/

Done.

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try_test.go
File utils/parallel/try_test.go (right):

https://codereview.appspot.com/42760043/diff/40001/utils/parallel/try_test.go...
utils/parallel/try_test.go:10: gc "launchpad.net/gocheck"
On 2013/12/17 11:06:37, dimitern wrote:
> please put gocheck on a separate line, after std imports

Done.