Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multierr.Errors is missing helper methods for error checking #302

Closed
adw1n opened this issue Mar 2, 2021 · 6 comments
Closed

multierr.Errors is missing helper methods for error checking #302

adw1n opened this issue Mar 2, 2021 · 6 comments

Comments

@adw1n
Copy link
Contributor

adw1n commented Mar 2, 2021

Goka processors return errors of type multierr.Errors. Users would like to be able to programmatically check the error types e.g. using errors.Is(err, context.Canceled) to find out if the error is expected or not. Based off of the error type users will use different logging level - e.g. info for expected context.Canceled errors and error for unexpected errors. Currently there is no way to retrieve any of the errors from multierr.Errors.errs.

Is there really a need to have the multierr package in goka? Can't we use something like https://github.com/hashicorp/go-multierror that already has all the bells and whistles?

@adw1n adw1n changed the title multierr.Errors is missing Unwrap & WrappedErrors methods for error checking multierr.Errors is missing helper methods for error checking Mar 4, 2021
@frairon
Copy link
Contributor

frairon commented Mar 10, 2021

Hey @adw1n,
It looks like replacing multierr/Errors with the library you suggested would solve the issue to get underlying errors, right? If that's the case and you have some time to spare, feel free to replace the errors - always happy to accept PRs :). I'm really not sure why we added our implementation of Errors in the first place.

For the multierr-package in general: on first glance, it also looks like multierr/ErrGroup could also be replaced with the errgroup from x/sync. Main difference however is, that this group returns only the first error, not any following, as can be seen here.
In order to get all erorrs including any shutdown-errors, I'd prefer to keep goka's builtin multierr/ErrGroup implementation which reeturns all of them. Unless I'm missing something and there's an actual alternative.

@frairon
Copy link
Contributor

frairon commented Mar 11, 2021

Just checked hashicorp/go-multierror again, and one thing struck out to me: there is no method to simply accumulate errors into the existing multi-Error like here where you can do

errs := new(multierr.Errors)
errs.Collect(foo())
errs.Collect(bar())

the one from hashicorp and others require to overwrite the accumulator like this:

var errs *multierr.Errors
errs = errs.Append(foo())
errs = errs.Append(bar())

Which version looks better is probably matter of taste, and the goka/multierr could use some more convenience functions. So I'm not sure what would be easier to do: extend goka/multierr with the needed functionality or replace it with some other library.
But as usual, open for suggestions.. let's see who's first :)

@db7
Copy link
Collaborator

db7 commented Mar 12, 2021

Indeed, the reason to write multierr in first place was the accumulation of errors.

@frairon frairon mentioned this issue Sep 17, 2021
@frairon
Copy link
Contributor

frairon commented Sep 30, 2021

@adw1n, it's been a while but it turned out that it's indeed better to use hashicorp's implementation for Errors.
Using Append instead of Collect isn't that bad after all :)
This PR will replace the usage in goka, as well with some other fixes. Have a look if you like.

@frairon
Copy link
Contributor

frairon commented Oct 4, 2021

it's been released to version v1.1.0-beta.1

@frairon
Copy link
Contributor

frairon commented Oct 19, 2021

I'll close this, as the underlying issue was basically fixed. Feel free to reopen or create a new issue if you need to.

@frairon frairon closed this as completed Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants