cckellogg commented on a change in pull request #517:
URL: https://github.com/apache/pulsar-client-go/pull/517#discussion_r631928139
##########
File path: pulsar/consumer_regex.go
##########
@@ -383,11 +382,14 @@ func subscriber(c *client, topics []string, opts
ConsumerOptions, ch chan Consum
return consumerErrorCh
}
-func filterTopics(topics []string, regex *regexp.Regexp) []string {
+func filterTopics(topics []string, regex *regexp.Regexp) ([]string, error) {
matches := make(map[string]bool)
matching := make([]string, 0)
for _, t := range topics {
- tn, _ := internal.ParseTopicName(t)
+ tn, err := internal.ParseTopicName(t)
Review comment:
I'm not sure if this good thing to error out here. The list of topic
passed here usually comes from a response from the broker, right?
##########
File path: pulsar/test_helper.go
##########
@@ -153,15 +153,24 @@ func deleteTopic(topic string) error {
func topicStats(topic string) (map[string]interface{}, error) {
var metadata map[string]interface{}
- err := httpGet("admin/v2/persistent/"+topicPath(topic)+"/stats",
&metadata)
+ tp, err := topicPath(topic)
+ if err != nil {
Review comment:
Not sure if this is needed since it's a test helper method. The test
should fail without this check if a bad topic name is passed. Same for the
topicPath function.
##########
File path: pulsar/producer_impl.go
##########
@@ -88,12 +88,17 @@ func newProducer(client *client, options *ProducerOptions)
(*producer, error) {
options.BatchingMaxPublishDelay = defaultBatchingMaxPublishDelay
}
+ ms, err := client.metrics.GetTopicMetrics(options.Topic)
Review comment:
This seems like a strange way of testing the topic. I don't think we
should rely on calling a metrics function. Why not just call `
internal.ParseTopicName(topic)`. Same for the consumer_impl and tests.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
