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

fix(kafka): Handle Sarama errors properly #3452

Merged
merged 2 commits into from
Aug 2, 2022

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Jul 31, 2022

Signed-off-by: Jorge Turrado jorge_turrado@hotmail.es

This PR adds extra traces to log Sarama errors but even they are errors that we aren't "catching" I haven't modified the behaviour to avoid breaking changes introduced by different error handling.

Checklist

Fixes #3056

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer JorTurFer requested a review from a team as a code owner July 31, 2022 19:07
@JorTurFer
Copy link
Member Author

Should I return them as regular errors and break the cycle instead of maintaining current behaviour?
WDYT @zroubalik ?

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 31, 2022

/run-e2e kafka*
Update: You can check the progress here

@tomkerkhove tomkerkhove changed the title **Kafka Scaler:** Handle Sarama errors properly fix(kafka): Handle Sarama errors properly Aug 1, 2022
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think you can try to get more detailed errors from the client creation?
Usually when there is a problem in conecting to Kafka Broker, there is just a generic error message stating, that client is not able to connect. Would be nice if we could get more detailed error info there.

@zroubalik
Copy link
Member

Should I return them as regular errors and break the cycle instead of maintaining current behaviour? WDYT @zroubalik ?

As a first step, I'd try to get as much (error) information as possible without breaking the current behaviour.

@JorTurFer
Copy link
Member Author

Do you think you can try to get more detailed errors from the client creation? Usually when there is a problem in conecting to Kafka Broker, there is just a generic error message stating, that client is not able to connect. Would be nice if we could get more detailed error info there.

checking

@JorTurFer
Copy link
Member Author

JorTurFer commented Aug 2, 2022

AFAI can see, we can't get more info because client creation doesn't bring any KError, so we only can check the standard error.
I have tried creating an authorized broker and connecting KEDA without credentials and I can see the authentication errors (from Kerror) on the first call to the broker (not in the client creation but in the call to DescribeTopics)

@zroubalik
Copy link
Member

AFAI can see, we can't get more info because client creation doesn't bring any KError, so we only can check the standard error. I have tried creating an authorized broker and connecting KEDA without credentials and I can see the authentication errors (from Kerror) on the first call to the broker (not in the client creation but in the call to DescribeTopics)

ack, thanks!

@zroubalik
Copy link
Member

@JorTurFer please fix changelog and merge :)

@JorTurFer JorTurFer enabled auto-merge (squash) August 2, 2022 14:58
@JorTurFer
Copy link
Member Author

JorTurFer commented Aug 2, 2022

/run-e2e kafka*
Update: You can check the progress here

@JorTurFer JorTurFer merged commit 8bc4c4a into kedacore:main Aug 2, 2022
@JorTurFer JorTurFer deleted the sarama-logs branch August 2, 2022 16:26
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

Successfully merging this pull request may close these issues.

Kafka scaler doesn't handle Sarama errors properly
2 participants