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:19:25 UTC
[GitHub] [pulsar] yangl opened a new pull request #10902: Correct TopicName#getPartitionIndex implementation.
yangl opened a new pull request #10902:
URL: https://github.com/apache/pulsar/pull/10902
### Motivation
Current partitioned topic check logic is not rigorous enough, such as the `mytopic-partition--1` this should not partitioned topic.
For details of the discussion see http://mail-archives.apache.org/mod_mbox/pulsar-dev/202106.mbox/browser
### Modifications
1. change the `getPartitionIndex` logic.
--
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
[GitHub] [pulsar] yangl commented on a change in pull request #10902: Correct TopicName#getPartitionIndex implementation.
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [pulsar] yangl commented on pull request #10902: Correct TopicName#getPartitionIndex implementation.
Posted by GitBox <gi...@apache.org>.
yangl commented on pull request #10902:
URL: https://github.com/apache/pulsar/pull/10902#issuecomment-861110056
> Could you please also remove the unused `log` field in `TopicName`? Just apply following diff.
>
> ```java
> diff --git a/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java b/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
> index bf6db5ff147..2cb9f23153b 100644
> --- a/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
> +++ b/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
> @@ -29,16 +29,12 @@ import java.util.concurrent.ExecutionException;
> import java.util.concurrent.TimeUnit;
> import org.apache.commons.lang3.StringUtils;
> import org.apache.pulsar.common.util.Codec;
> -import org.slf4j.Logger;
> -import org.slf4j.LoggerFactory;
>
> /**
> * Encapsulate the parsing of the completeTopicName name.
> */
> public class TopicName implements ServiceUnitId {
>
> - private static final Logger log = LoggerFactory.getLogger(TopicName.class);
> -
> public static final String PUBLIC_TENANT = "public";
> public static final String DEFAULT_NAMESPACE = "default";
>
> ```
Done.
--
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
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #10902: Correct TopicName#getPartitionIndex implementation.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #10902:
URL: https://github.com/apache/pulsar/pull/10902#discussion_r649869295
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
##########
@@ -272,7 +272,13 @@ 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);
Review comment:
Should we check `partitionIndex >= 0` and return -1 as the error index? You can add a test case for `mytopic-partition--2`.
##########
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:
Should we remove the warn logs? IMO, `getPartitionIndex` may be used to check if the topic is partitioned, if the topic is just expected to be a non-partitioned topic, the warn log is redundant.
##########
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!");
+ } else if (StringUtils.length(idx) != String.valueOf(partitionIndex).length()) {
+ // for the "topic-partition-01"
+ partitionIndex = -1;
+ log.warn("Partition index cannot start with a prefix of `0` unless it is 0!");
Review comment:
The same as I said below.
##########
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!");
+ } else if (StringUtils.length(idx) != String.valueOf(partitionIndex).length()) {
+ // for the "topic-partition-01"
+ partitionIndex = -1;
+ log.warn("Partition index cannot start with a prefix of `0` unless it is 0!");
Review comment:
The same as I said before.
##########
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:
But I still don't know why is the warn logs necessary? If user called `TopicName.get("my-topic-partition-xxx")`, then he'll get a line of warn logs. `TopicName` is a class of `pulsar-common` module that we should let the caller to determine whether to log something.
##########
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:
IMO, no logs is needed here.
Logging is used for helping users know what happened while the program is running. But for the package that is designed for being called by other packages, usually named `utils` or `common`, we should use exceptions or returned values to expose the running state so that caller side can know what happened. Usually, in these utils packages, a method should not contain too many steps whose internal state is invisible to caller side so logs is needed.
You can see these packages in many places of Pulsar. For example, the `org.apache.pulsar.common.protocol.Commands` class. Only `peekMessageMetadata` and `peekStickyKey` have error logs. It's because they want to swallow the exceptions and don't want callers know anything about it. However, IMO, these designs are **bad**. It actually swallows exceptions, then adds an error log, finally returns null. Unfortunately, the caller side doesn't check null value and NPE may happen. Even if we handled the null returned value, what should we do?
```java
MessageMetadata msgMetadata = Commands.peekMessageMetadata(metadataAndPayload, subscription.toString(), -1);
if (msgMetadata == null) {
// TODO: what logs should we print?
}
```
The good design is just to propagate the exception to caller side.
```java
try {
MessageMetadata msgMetadata = Commands.peekMessageMetadata(metadataAndPayload, subscription.toString(), -1);
// ...
} catch (SomeException e) {
// TODO: print logs from caller side
}
```
You can also take `org.apache.pulsar.common.net.ServiceURI` as example, no logs is used here and `IllegalArgumentException` will be thrown for fast fail.
Let's talk back to `TopicName#getPartitionIndex`. From caller side, we can check if the returned value equals to -1 so that we can know if the topic name is a non-partitioned topic, otherwise what's the partition. Your warn logs is just for printing the reason why it's a non-partitioned topic. However, caller never cares about the reason. Otherwise, it should be designed to:
```java
/**
* @return partition index of the completeTopicName.
* @throws exception if the topic is not a partitioned topic
*/
public static int getPartitionIndex(String topic) {
```
However, let's see the caller side. For example, in `org.apache.pulsar.broker.service.Consumer`'s constructor, it's used to initialize the `partitionIdx` field.
```java
this.partitionIdx = TopicName.getPartitionIndex(topicName);
```
What if the topic is a non-partitioned topic? Should we print any logs if it is? Even if the answer is yes, we only needs to print the topic name in the logs and this behavior should not be done by `getPartitionIndex`.
##########
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:
BTW, the warn logs of `catch (NumberFormatException nfe)` is redundant. For example, if a user just created a topic named `my-topic-partition-not-number`, and each time this topic is checked for whether it's a partitioned topic, a warn log will be logged though no one cares about it.
##########
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:
BTW, the warn logs of `catch (NumberFormatException nfe)` is also redundant. For example, if a user just created a topic named `my-topic-partition-not-number`, and each time this topic is checked for whether it's a partitioned topic, a warn log will be logged though no one cares about it.
##########
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:
No debug logs.
Debug logs is not intended to cover all your branches and should not be abused as well. If you add logs just to ensure that the logic goes here, for example, the `TopicName` constructor may be like:
```java
if (!completeTopicName.contains("://")) {
log.debug("completeTopicName '{}' doesn't contains '://'", completeTopicName);
// ...
}
// ...
if (parts.size() == 3) {
log.debug("parts.size() is 3");
// ...
} else if (parts.size() == 4) {
log.debug("parts.size() is 3");
// ...
} else {
log.debug("parts.size() is {}, not 3 or 4", parts.size());
throw new IllegalArgumentException("Invalid topic name: " + completeTopicName);
}
```
For a util method, use IDE's breakpoint for debugging. If you preferred logs, it's okay, just remove them after unit tests are added to cover all branches that have been logged.
##########
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:
No debug logs.
Debug logs is not intended to cover all your branches and should not be abused as well. If you add logs just to ensure that the logic goes here, for example, the `TopicName` constructor may be like:
```java
if (!completeTopicName.contains("://")) {
log.debug("completeTopicName '{}' doesn't contains '://'", completeTopicName);
// ...
}
// ...
if (parts.size() == 3) {
log.debug("parts.size() is 3");
// ...
} else if (parts.size() == 4) {
log.debug("parts.size() is 4");
// ...
} else {
log.debug("parts.size() is {}, not 3 or 4", parts.size());
throw new IllegalArgumentException("Invalid topic name: " + completeTopicName);
}
```
For a util method, use IDE's breakpoint for debugging. If you preferred logs, it's okay, just remove them after unit tests are added to cover all branches that have been logged.
##########
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:
And for your logs
```
log.warn("Partition index should >=0!");
```
Why **should**? When we pass a topic to `getPartitionIndex`, we should not assume it's a partitioned topic. Otherwise the method should be named like `getIndexOfPartitionedTopic`.
--
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
[GitHub] [pulsar] yangl commented on pull request #10902: Correct TopicName#getPartitionIndex implementation.
Posted by GitBox <gi...@apache.org>.
yangl commented on pull request #10902:
URL: https://github.com/apache/pulsar/pull/10902#issuecomment-859962530
--
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
[GitHub] [pulsar] BewareMyPower edited a comment on pull request #10902: Correct TopicName#getPartitionIndex implementation.
Posted by GitBox <gi...@apache.org>.
BewareMyPower edited a comment on pull request #10902:
URL: https://github.com/apache/pulsar/pull/10902#issuecomment-860692080
Could you please also remove the unused `log` field in `TopicName`? Just apply following diff.
```java
diff --git a/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java b/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
index bf6db5ff147..2cb9f23153b 100644
--- a/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
+++ b/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
@@ -29,16 +29,12 @@ import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import org.apache.commons.lang3.StringUtils;
import org.apache.pulsar.common.util.Codec;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* Encapsulate the parsing of the completeTopicName name.
*/
public class TopicName implements ServiceUnitId {
- private static final Logger log = LoggerFactory.getLogger(TopicName.class);
-
public static final String PUBLIC_TENANT = "public";
public static final String DEFAULT_NAMESPACE = "default";
```
--
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
[GitHub] [pulsar] BewareMyPower merged pull request #10902: Correct TopicName#getPartitionIndex implementation.
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged pull request #10902:
URL: https://github.com/apache/pulsar/pull/10902
--
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
[GitHub] [pulsar] BewareMyPower commented on pull request #10902: Correct TopicName#getPartitionIndex implementation.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #10902:
URL: https://github.com/apache/pulsar/pull/10902#issuecomment-860692080
Could you please also remove the unused `log` variable in `TopicName`? Just apply following diff.
```java
diff --git a/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java b/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
index bf6db5ff147..2cb9f23153b 100644
--- a/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
+++ b/pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
@@ -29,16 +29,12 @@ import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import org.apache.commons.lang3.StringUtils;
import org.apache.pulsar.common.util.Codec;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* Encapsulate the parsing of the completeTopicName name.
*/
public class TopicName implements ServiceUnitId {
- private static final Logger log = LoggerFactory.getLogger(TopicName.class);
-
public static final String PUBLIC_TENANT = "public";
public static final String DEFAULT_NAMESPACE = "default";
```
--
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