Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #2330: change getTopicName in MultiTopicsConsumer #2346

Merged
merged 3 commits into from Aug 27, 2018
Merged

Issue #2330: change getTopicName in MultiTopicsConsumer #2346

merged 3 commits into from Aug 27, 2018

Conversation

jiazhai
Copy link
Member

@jiazhai jiazhai commented Aug 9, 2018

Motivation

getTopicName in TopicMessageIdImpl and TopicMessageImpl is some kind of confusing.
Developer may mis-use it.

Modifications

change method getTopicName to let it return topic name without partition.
add method getTopicPartitionName, which return topic name with partition part.

Result

More clear of the method names.
Unit-Tests all pass.
Issue #2330 get fixed.

@srkukarni
Copy link
Contributor

rerun java tests

sijie
sijie reviewed Aug 9, 2018
View reviewed changes
pulsar-client/src/main/java/org/apache/pulsar/client/impl/TopicMessageImpl.java Outdated
* @return the name of the topic on which this message was published
*/
public String getTopicName() {
int position = topicName.lastIndexOf(PARTITIONED_TOPIC_SUFFIX);
checkState(position != -1, "Topic Name not contains partition part. " + topicName);
return topicName.substring(0, position);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think this is a bit expensive, if people calls getTopicName, it has to do substring on every call. Can we pass in topicName and topicPartitionName on constructing TopicMessageImpl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sijie Thanks. will keep the topicPartitionName while constructing.

@jiazhai jiazhai force-pushed the issue_2330 branch 3 times, most recently from 3688e16 to 85ba903 Compare August 11, 2018 09:12
merlimat
merlimat reviewed Aug 12, 2018
View reviewed changes
pulsar-client/src/main/java/org/apache/pulsar/client/impl/TopicMessageIdImpl.java Outdated
this.topicName = topicPartitionName.substring(0, position);
} else {
this.topicName = topicPartitionName;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will potentially create strings for each received message

sijie
sijie reviewed Aug 12, 2018
View reviewed changes
pulsar-client/src/main/java/org/apache/pulsar/client/impl/TopicMessageIdImpl.java Outdated

int position = topicPartitionName.lastIndexOf(PARTITIONED_TOPIC_SUFFIX);
if (position != -1) {
this.topicName = topicPartitionName.substring(0, position);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiazhai I think you misunderstood my comment.

I think a better solution is in ConsumerBase, we maintain two variables, one is topicName, the other is topicPartitionName. so when we construct TopicMessageIdImpl, it will use the strings constructed in ConsumerBase.

@jiazhai jiazhai force-pushed the issue_2330 branch 2 times, most recently from 591d9fd to 34b1565 Compare August 16, 2018 13:43
@jiazhai
Copy link
Member Author

jiazhai commented Aug 17, 2018 •
edited

@sijie @merlimat Thanks for the comments.
@sijie kept the shorter topic name without partition in ConsumerImpl instead of ConsumerBase, because ConsumerBase also a parent for MultiTopicsConsumer, which not has a meaningful topic name. and it is only used in ConsumerImpl.

@sijie sijie added type/bug The PR fixed a bug or issue reported a bug area/client labels Aug 17, 2018
@sijie sijie added this to the 2.2.0-incubating milestone Aug 17, 2018
@sijie
Copy link
Member

sijie commented Aug 17, 2018

@merlimat can you review this PR?

@sijie
Copy link
Member

sijie commented Aug 21, 2018

@jiazhai can you rebase this PR to master?

@merlimat can you review it again? we need this for 2.1.1 release

@sijie
Copy link
Member

sijie commented Aug 27, 2018

@jiazhai can you rebase this PR?

@sijie sijie merged commit 8688b67 into apache:master Aug 27, 2018
sijie pushed a commit that referenced this pull request Aug 27, 2018
@jiazhai @sijie
Issue #2330: change getTopicName in MultiTopicsConsumer (#2346)
2595941 
* change getTopicName in MultiTopicsConsumer

* change following sijie's comments

* keep both topicName and topicPartitonName in consumer to avoid new string
@sijie
Copy link
Member

sijie commented Aug 27, 2018

merged as 2595941 into branch-2.1

@sijie sijie mentioned this pull request Sep 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat left review comments

@sijie sijie sijie approved these changes

@srkukarni srkukarni srkukarni approved these changes

Assignees

@jiazhai jiazhai

Labels
area/client type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.1.1-incubating
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jiazhai @srkukarni @sijie @merlimat