[pulsar-client] Avoid subscribing the same topic again #7823
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
codelipenghui
requested review from
merlimat,
codelipenghui,
jiazhai and
sijie
| } | ||
|
|
||
| // subscribe one more given topic | ||
| public CompletableFuture<Void> subscribeAsync(String topicName, boolean createTopicIfDoesNotExist) { | ||
| if (!topicNameValid(topicName)) { | ||
| TopicName topicNameInstance = getTopicName(topicName); |
There was a problem hiding this 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.
There was a problem hiding this comment.
I validate it in line 746 to 748. Because TopicName could be reused, if I wrap the validation in topicNameValid, I have to create TopicName to get full topic name and partitioned topic name again. I can't think an elegant way to do it so I create a TopicName instance at first of subscribeAsync and do checks
There was a problem hiding this comment.
@codelipenghui Sorry I haven't noticed the later part of your review before. I didn't change the original behavior in this PR because it needs relative changes with unit tests. I'll do it soon.
Comment on lines
+723
to
+740
| 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); | ||
| } |
There was a problem hiding this comment.
It seems that we do not need to add these methods?
There was a problem hiding this comment.
getTopicName and getFullTopicName are just for ignoring the exceptions because we will throw a PulsarClientException later. removeTopic is to ensure that the key to remove is a full topic name.
|
/pulsarbot run-failure-checks |
jiazhai
approved these changes
Aug 17, 2020
codelipenghui
approved these changes
Aug 17, 2020
|
/pulsarbot run-failure-checks |
huangdx0726
pushed a commit
to huangdx0726/pulsar
that referenced
this pull request
Aug 24, 2020
### Motivation The current key of `MultiTopicsConsumerImpl.topics` is the topic name passed by user. The `topicNameValid` method checks if the name is valid and `topics` doesn't contain the key. However, if a multi topics consumer subscribed a partition of a subscribed partitioned topic, `subscribeAsync` succeed and a new `ConsumerImpl` of the same partition was created, which is redundant. Also, if a multi topics consumer subscribed `public/default/topic` or `persistent://public/default/topic`, while the initial subscribed topic is `topic`, the redundant consumers would be created. ### Modifications - Use full topic name as key of `MultiTopicsConsumerImpl.topics` - Check both full topic name and full partitioned topic name not exist in `MultiTopicsConsumerImpl.topics` when `subscribeAsync` is called - Throw a different exception to differ topic is invalid and topic is already subscribed - Add a unit test for subscribing a partition of a subscribed partitioned topic or the same topic with prefix
lbenc135
pushed a commit
to lbenc135/pulsar
that referenced
this pull request
Sep 5, 2020
### Motivation The current key of `MultiTopicsConsumerImpl.topics` is the topic name passed by user. The `topicNameValid` method checks if the name is valid and `topics` doesn't contain the key. However, if a multi topics consumer subscribed a partition of a subscribed partitioned topic, `subscribeAsync` succeed and a new `ConsumerImpl` of the same partition was created, which is redundant. Also, if a multi topics consumer subscribed `public/default/topic` or `persistent://public/default/topic`, while the initial subscribed topic is `topic`, the redundant consumers would be created. ### Modifications - Use full topic name as key of `MultiTopicsConsumerImpl.topics` - Check both full topic name and full partitioned topic name not exist in `MultiTopicsConsumerImpl.topics` when `subscribeAsync` is called - Throw a different exception to differ topic is invalid and topic is already subscribed - Add a unit test for subscribing a partition of a subscribed partitioned topic or the same topic with prefix
lbenc135
pushed a commit
to lbenc135/pulsar
that referenced
this pull request
Sep 5, 2020
### Motivation The current key of `MultiTopicsConsumerImpl.topics` is the topic name passed by user. The `topicNameValid` method checks if the name is valid and `topics` doesn't contain the key. However, if a multi topics consumer subscribed a partition of a subscribed partitioned topic, `subscribeAsync` succeed and a new `ConsumerImpl` of the same partition was created, which is redundant. Also, if a multi topics consumer subscribed `public/default/topic` or `persistent://public/default/topic`, while the initial subscribed topic is `topic`, the redundant consumers would be created. ### Modifications - Use full topic name as key of `MultiTopicsConsumerImpl.topics` - Check both full topic name and full partitioned topic name not exist in `MultiTopicsConsumerImpl.topics` when `subscribeAsync` is called - Throw a different exception to differ topic is invalid and topic is already subscribed - Add a unit test for subscribing a partition of a subscribed partitioned topic or the same topic with prefix
lbenc135
pushed a commit
to lbenc135/pulsar
that referenced
this pull request
Sep 5, 2020
### Motivation The current key of `MultiTopicsConsumerImpl.topics` is the topic name passed by user. The `topicNameValid` method checks if the name is valid and `topics` doesn't contain the key. However, if a multi topics consumer subscribed a partition of a subscribed partitioned topic, `subscribeAsync` succeed and a new `ConsumerImpl` of the same partition was created, which is redundant. Also, if a multi topics consumer subscribed `public/default/topic` or `persistent://public/default/topic`, while the initial subscribed topic is `topic`, the redundant consumers would be created. ### Modifications - Use full topic name as key of `MultiTopicsConsumerImpl.topics` - Check both full topic name and full partitioned topic name not exist in `MultiTopicsConsumerImpl.topics` when `subscribeAsync` is called - Throw a different exception to differ topic is invalid and topic is already subscribed - Add a unit test for subscribing a partition of a subscribed partitioned topic or the same topic with prefix
wolfstudy
pushed a commit
that referenced
this pull request
Oct 30, 2020
### Motivation The current key of `MultiTopicsConsumerImpl.topics` is the topic name passed by user. The `topicNameValid` method checks if the name is valid and `topics` doesn't contain the key. However, if a multi topics consumer subscribed a partition of a subscribed partitioned topic, `subscribeAsync` succeed and a new `ConsumerImpl` of the same partition was created, which is redundant. Also, if a multi topics consumer subscribed `public/default/topic` or `persistent://public/default/topic`, while the initial subscribed topic is `topic`, the redundant consumers would be created. ### Modifications - Use full topic name as key of `MultiTopicsConsumerImpl.topics` - Check both full topic name and full partitioned topic name not exist in `MultiTopicsConsumerImpl.topics` when `subscribeAsync` is called - Throw a different exception to differ topic is invalid and topic is already subscribed - Add a unit test for subscribing a partition of a subscribed partitioned topic or the same topic with prefix (cherry picked from commit e1b76a3)
merlimat
pushed a commit
to merlimat/pulsar
that referenced
this pull request
Dec 19, 2020
### Motivation The current key of `MultiTopicsConsumerImpl.topics` is the topic name passed by user. The `topicNameValid` method checks if the name is valid and `topics` doesn't contain the key. However, if a multi topics consumer subscribed a partition of a subscribed partitioned topic, `subscribeAsync` succeed and a new `ConsumerImpl` of the same partition was created, which is redundant. Also, if a multi topics consumer subscribed `public/default/topic` or `persistent://public/default/topic`, while the initial subscribed topic is `topic`, the redundant consumers would be created. ### Modifications - Use full topic name as key of `MultiTopicsConsumerImpl.topics` - Check both full topic name and full partitioned topic name not exist in `MultiTopicsConsumerImpl.topics` when `subscribeAsync` is called - Throw a different exception to differ topic is invalid and topic is already subscribed - Add a unit test for subscribing a partition of a subscribed partitioned topic or the same topic with prefix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The current key of
MultiTopicsConsumerImpl.topicsis the topic name passed by user. ThetopicNameValidmethod checks if the name is valid andtopicsdoesn't contain the key.However, if a multi topics consumer subscribed a partition of a subscribed partitioned topic,
subscribeAsyncsucceed and a newConsumerImplof the same partition was created, which is redundant.Also, if a multi topics consumer subscribed
public/default/topicorpersistent://public/default/topic, while the initial subscribed topic istopic, the redundant consumers would be created.Modifications
MultiTopicsConsumerImpl.topicsMultiTopicsConsumerImpl.topicswhensubscribeAsyncis called