top | item 34347176

(no title)

tmerr | 3 years ago

Ahh having a generic pool abstraction that collects results is tempting... nice work. I likely won't use this library though, since

  - I don't want to mess with panics.
  - The default concurrency GOMAXPROCS is almost never what I want.
  - Aggregated errors are almost never what I want. (I haven't read the implementation, but I worry about losing important error codes, or propagating something huge across an RPC boundary).
  - Using it would place a burden on any reader that isn't familiar with the library

discuss

order

camdencheek|3 years ago

> nice work

Thanks!

> The default concurrency GOMAXPROCS is almost never what I want.

FWIW, the default concurrency has been changed to "unlimited" since the 0.1.0 release.

> Aggregated errors are almost never what I want.

Out of curiousity, what do you want? There is an option to only keep the first error, and it's possible to unwrap the error to an array of errors that compose it if you just want a slice of errors.

> Using it would place a burden on any reader that isn't familiar with the library

Using concurrency in general places a burden on the reader :) I personally find using patterns like this to significantly reduce the read/review burden.

tmerr|3 years ago

> FWIW, the default concurrency has been changed to "unlimited" since the 0.1.0 release.

Nice! Will that end up on Github?

> Out of curiousity, what do you want

Most often I want to return just the first error. Some reasons: (1) smaller error messages passed across RPC boundaries (2) original errors can be inspected as intended (e.g. error codes) (3) when the semantics are to cancel after the first error, the errors that come after that are just noise.

A couple other thoughts

  - I think the non-conc comparison code is especially hairy since it's using goroutine pools. There's nothing wrong with that and it's fast, just not the easiest to work with. Often goroutine overhead is negligible and I would bound concurrency in dumber ways e.g. by shoving a sync.Semaphore into whatever code I otherwise have
  - I like that errgroup has .Go() block when concurrency limit is reached. Based on a quick skim of the code I think(?) conc does that too, but it could use documentation