[GitHub] [pulsar] codelipenghui commented on a change in pull request #7823: [pulsar-client] Avoid subscribing the same topic again

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

codelipenghui commented on a change in pull request #7823:
URL: https://github.com/apache/pulsar/pull/7823#discussion_r471189958



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImpl.java
##########
@@ -720,16 +720,36 @@ private void removeExpiredMessagesFromQueue(Set<MessageId> messageIds) {
         }
     }
 
-    private boolean topicNameValid(String topicName) {
-        return TopicName.isValid(topicName) && !topics.containsKey(topicName);
+    private TopicName getTopicName(String topic) {
+        try {
+            return TopicName.get(topic);
+        } catch (Exception ignored) {
+            return null;
+        }
+    }
+
+    private String getFullTopicName(String topic) {
+        TopicName topicName = getTopicName(topic);
+        return (topicName != null) ? topicName.toString() : null;
+    }
+
+    private void removeTopic(String topic) {
+        String fullTopicName = getFullTopicName(topic);
+        if (fullTopicName != null) {
+            topics.remove(topic);
+        }
     }
 
     // subscribe one more given topic
     public CompletableFuture<Void> subscribeAsync(String topicName, boolean createTopicIfDoesNotExist) {
-        if (!topicNameValid(topicName)) {
+        TopicName topicNameInstance = getTopicName(topicName);

Review comment:
       Why remove `!topicNameValid(topicName)` ? I think we should return `Topic name not valid` when the topic name does not valid and return `Already subscribe to {}` for the duplicate topic name.

##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/MultiTopicsConsumerImpl.java
##########
@@ -720,16 +720,36 @@ private void removeExpiredMessagesFromQueue(Set<MessageId> messageIds) {
         }
     }
 
-    private boolean topicNameValid(String topicName) {
-        return TopicName.isValid(topicName) && !topics.containsKey(topicName);
+    private TopicName getTopicName(String topic) {
+        try {
+            return TopicName.get(topic);
+        } catch (Exception ignored) {
+            return null;
+        }
+    }
+
+    private String getFullTopicName(String topic) {
+        TopicName topicName = getTopicName(topic);
+        return (topicName != null) ? topicName.toString() : null;
+    }
+
+    private void removeTopic(String topic) {
+        String fullTopicName = getFullTopicName(topic);
+        if (fullTopicName != null) {
+            topics.remove(topic);
+        }

Review comment:
       It seems that we do not need to add these methods?




----------------------------------------------------------------
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