[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request #517: Check ParseTopicName() return err

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org