Move multierror logic into luci-go.

Move multierror logic into luci-go.

R=dnj@chromium.org, estaab@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/luci-go/commit/24a854fe2dfcc698d9e7b8db137086b2162b7db5

[M-A Ruel, 2015-07-22 18:20:35]

maruel@chromium.org changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2015-07-22 18:20:36]

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go
File common/errors/errors.go (right):

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go...
common/errors/errors.go:14: // New is a pass-through version of the standard
errors.New function.
And what's the purpose?

[dnj, 2015-07-22 18:30:34]

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go
File common/errors/errors.go (right):

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go...
common/errors/errors.go:14: // New is a pass-through version of the standard
errors.New function.
On 2015/07/22 18:20:36, M-A Ruel wrote:
> And what's the purpose?

Guess: one can import "...luci-go/common/errors" and still use "errors.New" in
their code.

[M-A Ruel, 2015-07-22 18:32:36]

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go
File common/errors/errors.go (right):

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go...
common/errors/errors.go:14: // New is a pass-through version of the standard
errors.New function.
On 2015/07/22 18:30:34, dnj wrote:
> On 2015/07/22 18:20:36, M-A Ruel wrote:
> > And what's the purpose?
> 
> Guess: one can import "...luci-go/common/errors" and still use "errors.New" in
> their code.

Oh right, I forgot about the package name aliasing.

[iannucci, 2015-07-23 00:44:38]

is this OK to land or did ppl have comments?

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go
File common/errors/errors.go (right):

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go...
common/errors/errors.go:14: // New is a pass-through version of the standard
errors.New function.
On 2015/07/22 18:32:36, M-A Ruel wrote:
> On 2015/07/22 18:30:34, dnj wrote:
> > On 2015/07/22 18:20:36, M-A Ruel wrote:
> > > And what's the purpose?
> > 
> > Guess: one can import "...luci-go/common/errors" and still use "errors.New"
in
> > their code.
> 
> Oh right, I forgot about the package name aliasing.

Right... I figured this was easier than fixing the aliasing issue.

[dnj, 2015-07-23 01:03:37]

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go
File common/errors/errors.go (right):

https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go...
common/errors/errors.go:27: func (te *MarkedError) Error() string {
Move to markederror.go

[iannucci, 2015-07-23 01:13:25]

On 2015/07/23 01:03:37, dnj wrote:
> https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go
> File common/errors/errors.go (right):
> 
>
https://codereview.chromium.org/1249933002/diff/40001/common/errors/errors.go...
> common/errors/errors.go:27: func (te *MarkedError) Error() string {
> Move to markederror.go

Done

[dnj, 2015-07-23 01:14:22]

lgtm

[iannucci, 2015-07-23 01:14:40]

Committed patchset #4 (id:60001) manually as
24a854fe2dfcc698d9e7b8db137086b2162b7db5 (presubmit successful).

[M-A Ruel, 2015-08-10 19:00:21]

While the CL was a while ago, I'd like your thought on the followings.

https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
File common/errors/multierror.go (right):

https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
common/errors/multierror.go:84: Size int
I don't like that it is exported. I'd prefer to have a function:

func NewLazy(size int) *LazyMultiError {
  return &LazyMultiErrror{me: make(MultiError, 0, size)}
}

https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
common/errors/multierror.go:91: func (e *LazyMultiError) Assign(i int, err
error) {
Why not Add(err error) instead?

https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
common/errors/multierror.go:100: e.me[i] = err
That may crash quite unexpectedly at runtime, especially in rarely tested
failure case. Not a fan of this design.

https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
common/errors/multierror.go:115: func Fix(err error) error {
What's wrong with this instead?

if e, ok := err.(MultiError); ok {
  return e
}
return err

[iannucci, 2015-08-10 19:28:04]

On 2015/08/10 19:00:21, M-A Ruel wrote:
> While the CL was a while ago, I'd like your thought on the followings.

Much appreciated! :)

> 
>
https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
> File common/errors/multierror.go (right):
> 
>
https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
> common/errors/multierror.go:84: Size int
> I don't like that it is exported. I'd prefer to have a function:
> 
> func NewLazy(size int) *LazyMultiError {
>   return &LazyMultiErrror{me: make(MultiError, 0, size)}
> }
> 

Function sgtm, but the entire point of LazyMultiError is to avoid allocating a
potentially very long slice of errors (e.g. thousands), until some error
actually occurs.

I'll make a CL to make it a function instead.

>
https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
> common/errors/multierror.go:91: func (e *LazyMultiError) Assign(i int, err
> error) {
> Why not Add(err error) instead?

The point of this is to replace MultiError objects with a pre-known size. It
avoids allocating a potentially large []error unless one of the entries is
actually non-nil.

For cases where you don't know what the size is, you can just make a MultiError
and call append() on it directly.

> 
>
https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
> common/errors/multierror.go:100: e.me[i] = err
> That may crash quite unexpectedly at runtime, especially in rarely tested
> failure case. Not a fan of this design.

Again, the point of this class is for when you have a pre-known size. We
discussed recovery if you violate the contract, but decided a panic would be
best anyway.  The only other reasonable API would be to have e.me dynamically
size to fit i, but this would cause problems for the users of LazyMultiError,
which guarantee that the non-nil MultiError that they return has an exact
length. Functions like:

  DeleteMulti(keys []Key) error

Promise that if error != nil, it may be a MultiError, and if it's a MultiError,
it's length == len(keys).

> 
>
https://codereview.chromium.org/1249933002/diff/60001/common/errors/multierro...
> common/errors/multierror.go:115: func Fix(err error) error {
> What's wrong with this instead?
> 
> if e, ok := err.(MultiError); ok {
>   return e
> }
> return err

The Fix function is to cast other []error objects into a MultiError
(specifically the GAE SDK's appengine.MultiError, and the other GAE SDK's
appengine.MultiError). The thought here is that if the luci/gae library
documents that it returns a "luci-go/common/errors".MultiError, that it would be
that specific type (so that callers could use the construction you listed
above).

[M-A Ruel, 2015-08-10 19:33:35]

On 2015/08/10 19:28:04, iannucci wrote:
> On 2015/08/10 19:00:21, M-A Ruel wrote:
> > While the CL was a while ago, I'd like your thought on the followings.
> 
> Much appreciated! :)

Ok I understand better the use case now, thanks.