-
Notifications
You must be signed in to change notification settings - Fork 368
[feature] expose error to user when producer reconnect to broker failed #648
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
base: master
Are you sure you want to change the base?
Conversation
|
@yorkhellen Good patch, can you add test case for this change? |
cckellogg
left a comment
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.
What do other clients (like the java one) do in this situation? How are they notified?
| topic string | ||
| log log.Logger | ||
| cnx internal.Connection | ||
| err error |
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.
Why is this needed?
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.
hello @cckellogg , It seems that some of our Error messages are not fully exposed, such as sending failures after reaching the Backlog threshold. It seems that @yorkhellen is encapsulating their business based on the Go SDK, and may need to process specific business logic based on these specific errors. It seems that it is OK to expose the error field to the user here. This way the business can be more flexible in error handling.
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.
Can you provide some examples on the use case and or what the business logic will do with these errors? I'm just trying to understand better. Also, I think this should not be part of the 0.7.0 release so we can think about it more. If we are going to bubble some more errors up we should come up with a good api.
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.
Sure, I will provide a code example later to illustrate this matter.
Yes, we always include this change in 0.7.0
wolfstudy
left a comment
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 +1
|
cc @cckellogg PTAL again, thanks. |
| callback(nil, msg, errProducerClosed) | ||
| return | ||
| } | ||
| if p.err != nil { |
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.
This now changes the current behavior. Before if there was room the message would be added to the pending queue and sent when a new connection is established. Now the message will not be added. I'm not sure changing the default behavior is the correct approach here. Also, there could be a race condition checking the error and sending it since the the error is set in another go routine?
How do you envision your application handling this error?
Create producer failed will return error message to user. In some case broker notify producer close and producer can't reconnect broker successfully, but we can't get the error message when producer internal reconnect broker failed. We need handle error message. so expose error to user when producer reconnect to broker failed.