-
Notifications
You must be signed in to change notification settings - Fork 175
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
improving processor error handling/shutdown #353
Conversation
replace multierr.Errors with hashicorp/go-multierror
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (the error handling could cleanup in another PR, if possible at all)
@@ -314,7 +284,7 @@ func (g *Processor) Run(ctx context.Context) (rerr error) { | |||
g.log.Debugf("closing producer") | |||
defer g.log.Debugf("producer ... closed") | |||
if err := g.producer.Close(); err != nil { | |||
merrors.Collect(fmt.Errorf("error closing producer: %v", err)) | |||
errs = multierror.Append(errs, fmt.Errorf("error closing producer: %w", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the errs
here in this deferred func and then later using the errs
to fill the rerr
in the previous (but later called) deferred func still twists my head. This whole reversed defer processing makes this really hard to understand.
I think the code is correct, but if there is a cleaner way to write this, the code would be more maintainable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't like to modify the the return value in a deferred function, but haven't found a better solution yet. Maybe we'll find another way.
This PR fixes two main things: