asafm commented on code in PR #19235:
URL: https://github.com/apache/pulsar/pull/19235#discussion_r1111747832
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java:
##########
@@ -1284,7 +1289,11 @@ public void getPartitionedStats(
@ApiParam(value = "If return the earliest time in backlog")
@QueryParam("getEarliestTimeInBacklog") @DefaultValue("false")
boolean getEarliestTimeInBacklog) {
try {
- validatePartitionedTopicName(tenant, namespace, encodedTopic);
+ validateTopicName(tenant, namespace, encodedTopic);
Review Comment:
Got it.
1st, you need to change PR description. What you're basically doing here is
changing the validation.
Before: `-partition` couldn't appear anywhere in the partitioned topic name.
Now: `'partition-` can appear, but it is now allowed to be of suffix
`-partition-{num}`.
You're basically you're loosening up the validation.
2nd, you already have a method called `validatePartitionedTopicName` which
is supposed to validate the name of a partitioned topic. I believe you should
just modify the validation code in it, instead of keeping it as is and creating
another logic block containing that validation.
So take
```
protected void validatePartitionedTopicName(String tenant, String namespace,
String encodedTopic) {
// first, it has to be a validate topic name
validateTopicName(tenant, namespace, encodedTopic);
// second, "-partition-" is not allowed
if (encodedTopic.contains(TopicName.PARTITIONED_TOPIC_SUFFIX)) {
throw new RestException(Status.PRECONDITION_FAILED,
"Partitioned Topic Name should not contain '-partition-'");
}
}
```
And change last validation of partition to be `topicName` based as you did.
3rd, your exception is wrong. You say "Partitioned Topic Name should not
contain '-partition-`", but that was true *prior* to your change. It should
say, something like - "Only base topic name is allowed. Specify the same name
without `'-partition-{num}` for this to work"
Why? Because you know for a fact that if `isPartitioned()` is true, it means
it ends with `-partition-{num}`.
@eolivelli Shouldn't be lenient and actually fix the topic name to be
without the partition, since we actually know for sure it's a partition, so we
can fix it for the user?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
