-
Notifications
You must be signed in to change notification settings - Fork 345
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 panic when creating consumer with ReceiverQueueSize set to -1 #289
Conversation
@shohi Can you remove the comments from |
Or change the description? |
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.
The change LGTM, just little comments. please check
pulsar/consumer_impl.go
Outdated
// disable receiver queue if queue size is negative | ||
if options.ReceiverQueueSize < 0 { | ||
options.ReceiverQueueSize = 0 | ||
} |
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.
Maybe we can fix it using follows way:
if options.ReceiverQueueSize <= 0 {
options.ReceiverQueueSize = 1000
}
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.
unbuffered chan may be useful for some cases.
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.
not sure whether it's a good idea to remove this option. If negative value is totally disallowed (should be reflected in comments), i think it's ok to panic though.
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 is related to the flow mechanism of messages in pulsar. By default, the value of the receiveQueueSize is 1000
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.
Ok, i'll update the modification and comments according to your suggestions.
Yes. |
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.
Cool, thanks @shohi
Fixes #288