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

Improving some internal error-handling #846

merged 5 commits into from Apr 1, 2022


Copy link

@dominicbarnes dominicbarnes commented Feb 12, 2022

The goal of this PR is to set us up to improve visibility into errors being returned by kafka-go by wrapping those errors before bubbling them up throughout the call stack. The hope is that by making each error into a clear chain, it should be much easier to troubleshoot by narrowing in on the most relevant branch of code that is failing.

To start towards this more lofty goal, the first step (and the main focus of this PR) is to replace "strict" error-checking (eg: err == io.EOF) with more modern/flexible checks (eg: errors.Is(err, io.EOF)). This will allows us to freely wrap errors internally while not changing any error-handling logic at higher levels. Without this, blindly wrapping errors could result in subtle bugs as the older methods of checking errors would no longer behave the same way.

While I did include some error-wrapping changes, the changes are pretty focused on the error-checking instead, which is already a decent size PR. We can use subsequent PRs to refactor other components (eg: reader, writer) and catch any other error-checking places I may have missed.

@dominicbarnes dominicbarnes changed the title Improve visibility by wrapping errors with more context Improving some internal error-handling Mar 25, 2022
Copy link
Contributor Author

I had made some changes to conn.go, but it looks like the x/net/nettest package explicitly expects an unwrapped net.Error which causes the test to fail when a fmt.Errorf is used.

It's possible this is just an expectation of a net.Conn interface/contract we can't/shouldn't change, so we may have to tread more carefully there.

@dominicbarnes dominicbarnes marked this pull request as ready for review March 25, 2022 17:39
@achille-roussel achille-roussel merged commit ae86f55 into main Apr 1, 2022
@achille-roussel achille-roussel deleted the more-error-context branch April 1, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants