[fix][broker] Support getStats/update partitioned topic with `-partition-` #19235

yuruguo · 2023-01-15T06:05:45Z

Motivation

Same as #19230, We allow users to use the Client API to create the partitioned topic which name contains -partition-{not-number} when they enable partitioned type auto-creation.
But we didn't get stats and update it.

Modifications

Support getting stats and updating partitioned topics with the keyword -partition-{not-number}

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

codecov-commenter · 2023-01-15T06:27:29Z

Codecov Report

Merging #19235 (1bf2492) into master (fcecca4) will increase coverage by 29.58%.
The diff coverage is 66.66%.

@@              Coverage Diff              @@
##             master   #19235       +/-   ##
=============================================
+ Coverage     32.45%   62.04%   +29.58%     
- Complexity     6348    25705    +19357     
=============================================
  Files          1644     1844      +200     
  Lines        123737   135285    +11548     
  Branches      13494    14878     +1384     
=============================================
+ Hits          40162    83934    +43772     
+ Misses        77654    43599    -34055     
- Partials       5921     7752     +1831

Flag	Coverage Δ
inttests	`24.62% <44.44%> (-0.34%)`	⬇️
systests	`25.23% <50.00%> (-0.43%)`	⬇️
unittests	`59.19% <66.66%> (+41.64%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...in/java/org/apache/pulsar/admin/cli/CmdTopics.java	`80.16% <ø> (+80.16%)`	⬆️
...mon/policies/data/stats/SubscriptionStatsImpl.java	`74.74% <ø> (+32.32%)`	⬆️
...he/pulsar/broker/admin/v2/NonPersistentTopics.java	`61.92% <33.33%> (+34.14%)`	⬆️
...pache/pulsar/broker/admin/v2/PersistentTopics.java	`75.01% <33.33%> (+65.39%)`	⬆️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java	`77.99% <100.00%> (+37.41%)`	⬆️
...rg/apache/bookkeeper/mledger/impl/OpReadEntry.java	`72.44% <100.00%> (+5.43%)`	⬆️
...ker/service/persistent/PersistentSubscription.java	`68.69% <100.00%> (+22.32%)`	⬆️
...ion/buffer/metadata/TransactionBufferSnapshot.java	`42.85% <0.00%> (-42.86%)`	⬇️
...r/metadata/api/extended/MetadataStoreExtended.java	`33.33% <0.00%> (-16.67%)`	⬇️
...ker/authentication/AuthenticationProviderList.java	`42.55% <0.00%> (-8.56%)`	⬇️
... and 1252 more

HQebupt

LGTM

Technoboy- · 2023-01-30T08:30:04Z

It's better to discuss this on the dev mail list.

asafm · 2023-02-19T11:06:48Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java

@@ -810,6 +810,11 @@ public void updatePartitionedTopic(
            @ApiParam(value = "The number of partitions for the topic",
                    required = true, type = "int", defaultValue = "0")
                    int numPartitions) {
+        validateTopicName(tenant, namespace, encodedTopic);


I'm actually not following here.
You add a call to validateTopicName, but in line 818, you have a call to validatePartitionedTopicName which is:

// 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-'"); }

This seems to contain exactly the code you've added. So wham I missing here?

Sorry, I forget to delete line818(validatePartitionedTopicName) and has been fixed it.

asafm · 2023-02-19T11:07:37Z

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);


Same question. Seems like you inlined that method. Don't understand.

The validatePartitionedTopicName method has been deleted, so the above problem does not exist.

So now I can ask: What's the motivation for essentially copying the code from within validatePartitionedTopicName (which still exists in the code, as the method itself wasn't deleted) and pasting it in mutliple places?

The code I added is not exactly the same as the code you mentioned, mainly in topicName.isPartitioned() and encodedTopic.contains(TopicName.PARTITIONED_TOPIC_SUFFIX), the purpose is to improve the verification, details see #19234

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 ifisPartitioned()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?

…ion-`

asafm · 2023-02-20T10:21:51Z

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);


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 ifisPartitioned()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?

github-actions · 2023-03-23T01:55:05Z

The pr had no activity for 30 days, mark with Stale label.

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 15, 2023

yuruguo self-assigned this Jan 15, 2023

yuruguo added this to the 2.12.0 milestone Jan 15, 2023

yuruguo added area/broker ready-to-test labels Jan 15, 2023

yuruguo requested review from mattisonchao, coderzc, Technoboy- and codelipenghui January 15, 2023 06:10

HQebupt approved these changes Jan 17, 2023

View reviewed changes

yuruguo force-pushed the validate-partitioned-topic branch from 46cc314 to 2db9596 Compare January 29, 2023 02:43

asafm requested changes Feb 19, 2023

View reviewed changes

[fix][broker] Support getStats/update partitioned topic with `-partit…

1bf2492

…ion-`

yuruguo force-pushed the validate-partitioned-topic branch from 2db9596 to 1bf2492 Compare February 19, 2023 11:28

asafm requested changes Feb 20, 2023

View reviewed changes

github-actions bot added the Stale label Mar 23, 2023

poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023

yuruguo merged commit f4386c8 into apache:master Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Support getStats/update partitioned topic with `-partition-` #19235

[fix][broker] Support getStats/update partitioned topic with `-partition-` #19235

yuruguo commented Jan 15, 2023 •

edited

codecov-commenter commented Jan 15, 2023 •

edited

HQebupt left a comment

Technoboy- commented Jan 30, 2023

asafm Feb 19, 2023

yuruguo Feb 19, 2023 •

edited

asafm Feb 19, 2023

yuruguo Feb 19, 2023

asafm Feb 19, 2023

yuruguo Feb 20, 2023

asafm Feb 20, 2023

asafm Feb 20, 2023

github-actions bot commented Mar 23, 2023

[fix][broker] Support getStats/update partitioned topic with -partition- #19235

[fix][broker] Support getStats/update partitioned topic with -partition- #19235

Conversation

yuruguo commented Jan 15, 2023 • edited

Motivation

Modifications

Documentation

Matching PR in forked repository

codecov-commenter commented Jan 15, 2023 • edited

Codecov Report

HQebupt left a comment

Choose a reason for hiding this comment

Technoboy- commented Jan 30, 2023

asafm Feb 19, 2023

Choose a reason for hiding this comment

yuruguo Feb 19, 2023 • edited

Choose a reason for hiding this comment

asafm Feb 19, 2023

Choose a reason for hiding this comment

yuruguo Feb 19, 2023

Choose a reason for hiding this comment

asafm Feb 19, 2023

Choose a reason for hiding this comment

yuruguo Feb 20, 2023

Choose a reason for hiding this comment

asafm Feb 20, 2023

Choose a reason for hiding this comment

asafm Feb 20, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 23, 2023

[fix][broker] Support getStats/update partitioned topic with `-partition-` #19235

[fix][broker] Support getStats/update partitioned topic with `-partition-` #19235

yuruguo commented Jan 15, 2023 •

edited

codecov-commenter commented Jan 15, 2023 •

edited

yuruguo Feb 19, 2023 •

edited