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

Root cause of connection error not relayed to the caller #2128

Closed
k-wall opened this issue Feb 3, 2022 · 1 comment
Closed

Root cause of connection error not relayed to the caller #2128

k-wall opened this issue Feb 3, 2022 · 1 comment

Comments

@k-wall
Copy link
Contributor

k-wall commented Feb 3, 2022

Problem Description

Currently if a sarama based application fails to connect to the cluster, unless the application is happens to have sarama.Logger defined (which is quite chatty for production), the only error message seen in the logs is:

kafka: client has run out of available brokers to talk to (Is your cluster reachable?)

without any more detail. The useful detail of connection refused, connection timeout, certificate expiration is lost.

From an operational perspective (production environment), the makes root cause determination slow. The application that uses sarama needs to be reconfigured to get more information, which is inconvenient.

From what can tell from the sarama client, the underlying error is not propagated to the caller. Is that right?

I'm contemplating a PR to expose the underlying errors back to the caller. Using Go 1.13 wrapped error pattern seems attractive, but this would be an API change for any user with code checking sarama's sentianal error values (if error == ErrOutOfBrokers etc). How would such a PR be viewed?

Is there a less intrusive change?

@dnwe
Copy link
Collaborator

dnwe commented Feb 3, 2022

@k-wall thanks for raising this issue, I agree entirely and it's a good suggestion.

I think it would be fine for us to move to a model of Go 1.13 wrapped errors and simple document in our Changelog / release notes that there will be a breaking change whereby they must change any error comparison code from:

if err == sarama.ErrOutOfBrokers ==> if errors.Is(err, sarama.ErrOutOfBrokers)

We could also add some helpers to errors.go such as a func IsErrOutOfBrokers that does this for them if we think that would be useful, but I imagine most developers will be aware of the new errors.Is APIs by now.

k-wall added a commit to k-wall/sarama that referenced this issue Feb 7, 2022
…aluate error equality

refactoring only - no functionality changes
k-wall added a commit to k-wall/sarama that referenced this issue Feb 7, 2022
k-wall added a commit to k-wall/sarama that referenced this issue Feb 7, 2022
k-wall added a commit to k-wall/sarama that referenced this issue Feb 7, 2022
k-wall added a commit to k-wall/sarama that referenced this issue Feb 7, 2022
…aluate error equality

mechancial refactoring only - no functionality changes
k-wall added a commit to k-wall/sarama that referenced this issue Feb 7, 2022
k-wall added a commit to k-wall/sarama that referenced this issue Feb 7, 2022
…(s)s that prevented successful connection(s) to the brokers.
k-wall added a commit to k-wall/sarama that referenced this issue Feb 8, 2022
…aluate error equality

mechancial refactoring only - no functionality changes
k-wall added a commit to k-wall/sarama that referenced this issue Feb 8, 2022
…(s)s that prevented successful connection(s) to the brokers.
k-wall added a commit to k-wall/sarama that referenced this issue Feb 8, 2022
…(s) that prevented successful connection(s) to the brokers.
dnwe pushed a commit to k-wall/sarama that referenced this issue Feb 8, 2022
mechancial refactoring only - no functional changes

Fixes IBM#2128
dnwe pushed a commit to k-wall/sarama that referenced this issue Feb 8, 2022
Ensure that ErrOutOfBrokers exception(s) include the actual underlying
error that prevented successful connection(s) to the brokers.

Fixes IBM#2128
dnwe pushed a commit to k-wall/sarama that referenced this issue Feb 8, 2022
mechancial refactoring only - no functional changes

Fixes IBM#2128
dnwe pushed a commit to k-wall/sarama that referenced this issue Feb 8, 2022
Ensure that ErrOutOfBrokers exception(s) include the actual underlying
error that prevented successful connection(s) to the brokers.

Fixes IBM#2128
k-wall added a commit to k-wall/sarama that referenced this issue Feb 10, 2022
mechancial refactoring only - no functional changes

Fixes IBM#2128
k-wall added a commit to k-wall/sarama that referenced this issue Feb 10, 2022
Ensure that ErrOutOfBrokers exception(s) include the actual underlying
error that prevented successful connection(s) to the brokers.

Fixes IBM#2128
k-wall added a commit to k-wall/sarama that referenced this issue Feb 18, 2022
Ensure that ErrOutOfBrokers exception(s) include the actual underlying
error that prevented successful connection(s) to the brokers.

Fixes IBM#2128
k-wall added a commit to k-wall/sarama that referenced this issue Feb 18, 2022
mechancial refactoring only - no functional changes

Fixes IBM#2128
k-wall added a commit to k-wall/sarama that referenced this issue Feb 18, 2022
Ensure that ErrOutOfBrokers exception(s) include the actual underlying
error that prevented successful connection(s) to the brokers.

Fixes IBM#2128
k-wall added a commit to k-wall/sarama that referenced this issue Feb 18, 2022
mechancial refactoring only - no functional changes

Fixes IBM#2128
k-wall added a commit to k-wall/sarama that referenced this issue Feb 18, 2022
Ensure that ErrOutOfBrokers exception(s) include the actual underlying
error that prevented successful connection(s) to the brokers.

Fixes IBM#2128
dnwe pushed a commit to k-wall/sarama that referenced this issue Feb 24, 2022
Ensure that ErrOutOfBrokers exception(s) include the actual underlying
error that prevented successful connection(s) to the brokers.

Fixes IBM#2128
@dnwe dnwe closed this as completed in 8f626c8 Feb 24, 2022
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

2 participants