You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:28:31 UTC
[GitHub] [pulsar] yangl commented on a change in pull request #10902: Correct TopicName#getPartitionIndex implementation.
yangl commented on a change in pull request #10902:
URL: https://github.com/apache/pulsar/pull/10902#discussion_r650315155
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
##########
@@ -272,7 +272,17 @@ public static int getPartitionIndex(String topic) {
int partitionIndex = -1;
if (topic.contains(PARTITIONED_TOPIC_SUFFIX)) {
try {
- partitionIndex = Integer.parseInt(topic.substring(topic.lastIndexOf('-') + 1));
+ String idx = StringUtils.substringAfterLast(topic, PARTITIONED_TOPIC_SUFFIX);
+ partitionIndex = Integer.parseInt(idx);
+ if (partitionIndex < 0) {
+ // for the "topic-partition--1"
+ partitionIndex = -1;
+ log.warn("Partition index should >=0!");
Review comment:
The constructor method depends on `private TopicName(String completeTopicName)` and it depends on this method to set the `partitionIndex`, and the `isPartitioned()` method depends on the `partitionIndex`.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
##########
@@ -272,7 +272,17 @@ public static int getPartitionIndex(String topic) {
int partitionIndex = -1;
if (topic.contains(PARTITIONED_TOPIC_SUFFIX)) {
try {
- partitionIndex = Integer.parseInt(topic.substring(topic.lastIndexOf('-') + 1));
+ String idx = StringUtils.substringAfterLast(topic, PARTITIONED_TOPIC_SUFFIX);
+ partitionIndex = Integer.parseInt(idx);
+ if (partitionIndex < 0) {
+ // for the "topic-partition--1"
+ partitionIndex = -1;
+ log.warn("Partition index should >=0!");
Review comment:
Hmmm, maybe debug level is more appropriate? What do you think
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
##########
@@ -272,7 +272,17 @@ public static int getPartitionIndex(String topic) {
int partitionIndex = -1;
if (topic.contains(PARTITIONED_TOPIC_SUFFIX)) {
try {
- partitionIndex = Integer.parseInt(topic.substring(topic.lastIndexOf('-') + 1));
+ String idx = StringUtils.substringAfterLast(topic, PARTITIONED_TOPIC_SUFFIX);
+ partitionIndex = Integer.parseInt(idx);
+ if (partitionIndex < 0) {
+ // for the "topic-partition--1"
+ partitionIndex = -1;
+ log.warn("Partition index should >=0!");
Review comment:
Thanks for your explanation, but throwing an exception here may not be particularly appropriate, since this method is called when the topicname is instantiated to initialize the partionIndex.
I agree that the logs are not printed here because it doesn't matter if it's a partitioned topic or not. However, output a debug level log here for easy debugging.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
##########
@@ -272,7 +272,17 @@ public static int getPartitionIndex(String topic) {
int partitionIndex = -1;
if (topic.contains(PARTITIONED_TOPIC_SUFFIX)) {
try {
- partitionIndex = Integer.parseInt(topic.substring(topic.lastIndexOf('-') + 1));
+ String idx = StringUtils.substringAfterLast(topic, PARTITIONED_TOPIC_SUFFIX);
+ partitionIndex = Integer.parseInt(idx);
+ if (partitionIndex < 0) {
+ // for the "topic-partition--1"
+ partitionIndex = -1;
+ log.warn("Partition index should >=0!");
Review comment:
OK, deleted.
--
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