[client] rewrite regexConsumer to avoid duplicate code #848
Conversation
labuladong
force-pushed
the
rewrite-regex-consumer
branch
from
c1c811f to
0e1ccb0
Compare
labuladong
changed the title
pulsar/consumer_regex.go
Outdated
| func (c *regexConsumer) topics() ([]string, error) { | ||
| topics, err := c.client.lookupService.GetTopicsOfNamespace(c.namespace, internal.Persistent) | ||
| func topics(client *client, namespace string, pattern *regexp.Regexp) ([]string, error) { | ||
| topics, err := client.lookupService.GetTopicsOfNamespace(namespace, internal.Persistent) |
There was a problem hiding this comment.
It should check the client for nil.
There was a problem hiding this comment.
This is a private function, we ensure the client is not nil. If the client is nil, I think panic is acceptable.
There was a problem hiding this comment.
Can we make this function as part of struct method? So that client would not be nil.
func (c *regexConsumer) topics() ..{
topics, err: =c.client
pulsar/consumer_regex.go
Outdated
| func (c *regexConsumer) topics() ([]string, error) { | ||
| topics, err := c.client.lookupService.GetTopicsOfNamespace(c.namespace, internal.Persistent) | ||
| func topics(client *client, namespace string, pattern *regexp.Regexp) ([]string, error) { | ||
| topics, err := client.lookupService.GetTopicsOfNamespace(namespace, internal.Persistent) |
There was a problem hiding this comment.
Can we make this function as part of struct method? So that client would not be nil.
func (c *regexConsumer) topics() ..{
topics, err: =c.client
pulsar/consumer_regex.go
Outdated
| topics, err := topics(client, tn.Namespace, pattern) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| mtc, err := newMultiTopicConsumer(client, opts, topics, msgCh, dlq, rlq) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Can we make this function as part of struct method?
Hi @zzzming. In the original implementation, topics is a method of regexConsumer struct. In the new implementation, we need to get the topics before initializing regexConsumer. But I think make this topics function as a method of client struct will be better, I will work on this.
labuladong
force-pushed
the
rewrite-regex-consumer
branch
from
1a51790 to
4e9e33c
Compare
|
Conflict resolved, ready to merge. |
|
Ping @labuladong |
|
Conflict resolved. #805 does the same change in |
|
Can this pr be merged? @nodece @Gleiphir2769 @RobertIndie |
Fixes #846
Verifying this change
This change is already covered by existing tests, such as
consumer_regex_test.go.Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesModifications
Reuse
multiTopicConsumerinregexConsumerto avoid duplicate code.