You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/16 08:52:46 UTC
[GitHub] [pulsar-client-go] labuladong opened a new pull request, #848: [client] rewrite regexConsumer and fix unit test
labuladong opened a new pull request, #848:
URL: https://github.com/apache/pulsar-client-go/pull/848
Master Issue: #846
### Modifications
1. Reuse `multiTopicConsumer` in `regexConsumer`.
2. Fix test cases of `regexConsumer`. Due to the system topic `__change_events`, we can't compare the number of consumers to check the correctness:
Instead, I try to check the key in the `consumers` map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] labuladong commented on a diff in pull request #848: [client] rewrite regexConsumer to avoid duplicate code
Posted by GitBox <gi...@apache.org>.
labuladong commented on code in PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#discussion_r991802251
##########
pulsar/consumer_regex.go:
##########
@@ -37,73 +35,36 @@ const (
)
type regexConsumer struct {
- client *client
- dlq *dlqRouter
- rlq *retryRouter
-
- options ConsumerOptions
-
- messageCh chan ConsumerMessage
+ *multiTopicConsumer
namespace string
pattern *regexp.Regexp
consumersLock sync.Mutex
- consumers map[string]Consumer
subscribeCh chan []string
unsubscribeCh chan []string
-
- closeOnce sync.Once
- closeCh chan struct{}
-
- ticker *time.Ticker
-
- log log.Logger
-
- consumerName string
+ ticker *time.Ticker
}
-func newRegexConsumer(c *client, opts ConsumerOptions, tn *internal.TopicName, pattern *regexp.Regexp,
- msgCh chan ConsumerMessage, dlq *dlqRouter, rlq *retryRouter) (Consumer, error) {
+func newRegexConsumer(client *client, opts ConsumerOptions, tn *internal.TopicName, pattern *regexp.Regexp,
+ msgCh chan ConsumerMessage, dlq *dlqRouter, rlq *retryRouter) (*regexConsumer, error) {
+ 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
Review 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 initialize `regexConsumer`. But I think make this `topics` function as method of `client` struct will be better, I will work on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] labuladong commented on pull request #848: [client] rewrite regexConsumer and fix unit test
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#issuecomment-1249111116
cc @nodece @RobertIndie @wolfstudy
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] zzzming commented on a diff in pull request #848: [client] rewrite regexConsumer to avoid duplicate code
Posted by GitBox <gi...@apache.org>.
zzzming commented on code in PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#discussion_r991568581
##########
pulsar/consumer_regex.go:
##########
@@ -360,13 +235,13 @@ func (c *regexConsumer) unsubscribe(topics []string) {
}
}
-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)
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] liangyuanpeng commented on a diff in pull request #848: [client] rewrite regexConsumer to avoid duplicate code
Posted by GitBox <gi...@apache.org>.
liangyuanpeng commented on code in PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#discussion_r975523258
##########
pulsar/consumer_regex.go:
##########
@@ -360,13 +235,13 @@ func (c *regexConsumer) unsubscribe(topics []string) {
}
}
-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)
Review Comment:
It should check the `client` for `nil`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] nodece commented on pull request #848: [client] rewrite regexConsumer to avoid duplicate code
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#issuecomment-1330067486
Ping @labuladong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] labuladong commented on a diff in pull request #848: [client] rewrite regexConsumer to avoid duplicate code
Posted by GitBox <gi...@apache.org>.
labuladong commented on code in PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#discussion_r977225157
##########
pulsar/consumer_regex.go:
##########
@@ -360,13 +235,13 @@ func (c *regexConsumer) unsubscribe(topics []string) {
}
}
-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)
Review Comment:
This is a private function, we ensure the client is not nil. If the client is nil, I think panic is acceptable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] labuladong commented on a diff in pull request #848: [client] rewrite regexConsumer to avoid duplicate code
Posted by GitBox <gi...@apache.org>.
labuladong commented on code in PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#discussion_r991803110
##########
pulsar/consumer_regex.go:
##########
@@ -37,73 +35,36 @@ const (
)
type regexConsumer struct {
- client *client
- dlq *dlqRouter
- rlq *retryRouter
-
- options ConsumerOptions
-
- messageCh chan ConsumerMessage
+ *multiTopicConsumer
namespace string
pattern *regexp.Regexp
consumersLock sync.Mutex
- consumers map[string]Consumer
subscribeCh chan []string
unsubscribeCh chan []string
-
- closeOnce sync.Once
- closeCh chan struct{}
-
- ticker *time.Ticker
-
- log log.Logger
-
- consumerName string
+ ticker *time.Ticker
}
-func newRegexConsumer(c *client, opts ConsumerOptions, tn *internal.TopicName, pattern *regexp.Regexp,
- msgCh chan ConsumerMessage, dlq *dlqRouter, rlq *retryRouter) (Consumer, error) {
+func newRegexConsumer(client *client, opts ConsumerOptions, tn *internal.TopicName, pattern *regexp.Regexp,
+ msgCh chan ConsumerMessage, dlq *dlqRouter, rlq *retryRouter) (*regexConsumer, error) {
+ 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
+ }
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] labuladong commented on pull request #848: [client] rewrite regexConsumer to avoid duplicate code
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#issuecomment-1274124694
Conflict resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] labuladong commented on pull request #848: [client] rewrite regexConsumer to avoid duplicate code
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#issuecomment-1330143630
Can this pr be merged? @nodece @Gleiphir2769 @RobertIndie
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] labuladong commented on pull request #848: [client] rewrite regexConsumer to avoid duplicate code
Posted by GitBox <gi...@apache.org>.
labuladong commented on PR #848:
URL: https://github.com/apache/pulsar-client-go/pull/848#issuecomment-1330091162
Conflict resolved. [#805](https://github.com/apache/pulsar-client-go/pull/805/files#diff-541f3a2b60fd1d22abb2b35ce07048aecd7bdcf09e03d22cd5dd3c5d95c814bc) does the same change in `regexConsumer` and `multiTopicConsumer`, so this PR is still compatible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org