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:
[email protected]
