-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Support key-shared subscribe mode for go client #4465
Support key-shared subscribe mode for go client #4465
Conversation
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
@@ -156,7 +156,7 @@ func createProducerAsync(client *client, schema Schema, options ProducerOptions, | |||
C._pulsar_producer_configuration_set_message_router(conf, savePointer(&options.MessageRouter)) | |||
} | |||
|
|||
if options.Batching { | |||
if !options.Batching { |
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.
nice catch
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 should not depends on default values
pulsar-client-go/pulsar/consumer.go
Outdated
|
||
// Multiple consumer will be able to use the same subscription and all messages with the same key | ||
// will be dispatched to only one consumer | ||
keyShared |
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 should be start with capital letter too, like Exclusive
and Failover
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.
yes, should is KeyShared
pulsar-client-go/pulsar/consumer.go
Outdated
|
||
// Multiple consumer will be able to use the same subscription and all messages with the same key | ||
// will be dispatched to only one consumer | ||
keyShared |
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.
Should k be capitalized here? otherwise it's private to the package right?
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.
Thank you for reminding me.
@@ -156,7 +156,7 @@ func createProducerAsync(client *client, schema Schema, options ProducerOptions, | |||
C._pulsar_producer_configuration_set_message_router(conf, savePointer(&options.MessageRouter)) | |||
} | |||
|
|||
if options.Batching { | |||
if !options.Batching { |
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 a different issue which we should address separately. The main issue is that batching is just a flag, so there's no way to know wether the user touched it or not (eg: it should have been a *bool
instead).
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.
In cpp client, the default value of batch is true. So, only when batch is false, the user need to set it up.
(eg: it should have been a
*bool
instead)
yes, use this way, we can know wether the user touched it or not, by:
if options.Batching !=nil {
...
}
but the effect between them is the same.
When using it, I think users prefer to use it this way:
producer, err := client.CreateProducer(ProducerOptions{
Topic: topic,
Batching: false,
})
Instead of:
var flag = false
producer, err := client.CreateProducer(ProducerOptions{
Topic: topic,
Batching: &flag,
})
So, I don't think we need to care wether the user has batch
here.
Signed-off-by: xiaolong.ran <ranxiaolong716@gmail.com>
I removed the batch-related logic from the current pull request and modified the logic in a new pull request. PTAL thanks. Before you merge the current pull request, please merge pull/4475 |
run java8 tests |
Signed-off-by: xiaolong.ran ranxiaolong716@gmail.com
Motivation
Support key-shared subscribe mode for go client