[fix] [broker] Fix wrong logic of method TopicName.getPartition(int index) #19841

poorbarcode · 2023-03-16T14:24:45Z

Motivation

The method TopicName.getPartition(int index) has a bug which misidentifies "tp-partition-0-DLQ-partition-0" as "tp-partition-0-DLQ".

Modifications

Fix the wrong logic of the method TopicName.getPartition(int index)

(Highlight) Note: New versions containing this patch cannot be upgraded smoothly

1. If users use the feature DLQ and enable topic auto-creation, the DLQ will be auto-created. For example:

Consumer<String> consumer = pulsarClient.newConsumer(Schema.STRING)
                .topic("primary-tp-partition-0")
                .subscriptionName("sub1")
                .enableRetry(true)
                .subscribe();

These two topics will be auto-created:

primary-tp-partition-0-sub1-RETRY

2. And if users set the rule of the topic auto-creation like this:

allowAutoTopicCreationType=partitioned
defaultNumPartitions=2

Then will create two partitioned topics will be created:

primary-tp-partition-0-sub1-RETRY
- primary-tp-partition-0-sub1-RETRY-partition-0
- primary-tp-partition-0-sub1-RETRY-partition-1

3. (Highlight)And if hit current issue:

Then will create two partitioned topics will be created:

primary-tp-partition-0-sub1-RETRY
- primary-tp-partition-0-sub1-RETRY

Just show an example:

there has a partitioned topic named primary-tp.
so there will have a partition named primary-tp-partition-0.
if enabled the feature DLQ, we get DLQ primary-tp-partition-0-sub1-DLQ.
and if the DLQ is a partitioned topic, we get two partitions: primary-tp-partition-0-sub1-DLQ-partition-0 and primary-tp-partition-0-sub1-DLQ-partition-1

4. What happens when you hit the issue and upgrade to the new version

There have retry topics as section-3 before the upgrade, and consumers consume messages from the topic primary-tp-partition-0-sub1-RETRY.

These two partitions below will be created, and consumers will consume messages from these two topics below

primary-tp-partition-0-sub1-RETRY-partition-0
primary-tp-partition-0-sub1-RETRY-partition-1

But the messages in the original topic primary-tp-partition-0-sub1-RETRY will not be consumed anymore. It seems some messages were lost.

5. Workaround

Delete the ZK node primary-tp-partition-0-sub1-RETRY before the upgrade. Pulsar considers this topic to be non-partitioned, then the partitions will not be created after the upgrade.

If users hits the current issue before

there is a partitioned topic named

then the partition of the DLQ whose named *-partition-*-partition-* hits this issue.

6. (Highlight) Whether need a multi-partition DLQ/Retry Topic

See PIP 263:

Documentation

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

Matching PR in forked repository

PR in forked repository:

[ci] [fix] [broker] fix infinite http call getSubscriptions caused by wrong topicName poorbarcode/pulsar#78

wangjialing218 · 2023-03-21T03:29:13Z

This fix could solve #18149

poorbarcode · 2023-03-21T06:12:18Z

Hi @wangjialing218

This fix could solve #18149

Yes, I've seen the issue, and I'm sure it's the same one

poorbarcode · 2023-03-22T02:55:12Z

/pulsarbot rerun-failure-checks

codelipenghui

As most cases suffer loss from the DLQ topic name, can we just avoid the topic name with -partition- and -DLQ keyword run into the infinite loop? So that we will not break the current behavior.

In other words, we will not automatically create a partitioned topic for the DLQ topic even if users are enabled partitioned topic auto-creation. If users really need a partitioned topic for DLQ, they should create it themselves. IMO, it should be a misuse of DLQ, users should not set a high throughput expected to a DLQ. That doesn't make sense for most real cases.

codelipenghui · 2023-04-07T00:50:22Z

pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java

@@ -231,7 +231,7 @@ public String getEncodedLocalName() {
    }

    public TopicName getPartition(int index) {
-        if (index == -1 || this.toString().contains(PARTITIONED_TOPIC_SUFFIX)) {
+        if (index == -1 || this.toString().endsWith(PARTITIONED_TOPIC_SUFFIX + index)) {


What I thought was the PR intent to allow the partitioned name can contain -partition- keyword to resolve the issue. But it will introduce a new unreasonable rule to the partitioned topic.

we will not automatically create a partitioned topic for the DLQ topic even if users are enabled partitioned topic auto-creation.

Yes, PIP-263: Just auto-create no-partitioned DLQ And Prevent auto-create a DLQ for a DLQ try to do this.

What I thought was the PR intent to allow the partitioned name can contain -partition- keyword to resolve the issue. But it will introduce a new unreasonable rule to the partitioned topic.

I think there are three separate things:

Our rule for a partition name and how to check which index it is: end with -partition-x or contain -partition-x

The current logic is incorrect, and the current PR can fix it.

Whether to allow to create a topic whose name contains -paritition-

the behavior is denied for Admin API
-the behavior is allowed for client(such as create by a cmd-subscribe), you can see the tests testInfiniteHttpCallGetSubscriptions2, testInfiniteHttpCallGetSubscriptions3 in current PR

Whether only create non-partitioned topics for the Dead Letter or Retry Letter

PIP-263: Just auto-create no-partitioned DLQ And Prevent auto-create a DLQ for a DLQ try to do this.

poorbarcode · 2023-04-18T17:04:41Z

I have submitted a quick fix for this issue. Please take a look. @Technoboy- @codelipenghui

[fix] [broker] Fast fix infinite HTTP call getSubscriptions caused by wrong topicName #20131

codecov-commenter · 2023-04-23T05:09:31Z

Codecov Report

Merging #19841 (73cb806) into master (6036dcc) will increase coverage by 0.00%.
The diff coverage is 41.17%.

@@            Coverage Diff            @@
##             master   #19841   +/-   ##
=========================================
  Coverage     72.93%   72.93%           
- Complexity    31934    31963   +29     
=========================================
  Files          1868     1868           
  Lines        138449   138458    +9     
  Branches      15235    15236    +1     
=========================================
+ Hits         100978   100991   +13     
  Misses        29438    29438           
+ Partials       8033     8029    -4

Flag	Coverage Δ
inttests	`24.19% <0.00%> (+0.08%)`	⬆️
systests	`24.84% <0.00%> (-0.09%)`	⬇️
unittests	`72.22% <41.17%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...pulsar/broker/admin/impl/PersistentTopicsBase.java	`64.30% <33.33%> (-0.10%)`	⬇️
...mon/policies/data/stats/SubscriptionStatsImpl.java	`69.91% <50.00%> (-0.74%)`	⬇️
...ava/org/apache/pulsar/common/naming/TopicName.java	`96.33% <100.00%> (ø)`

... and 78 files with indirect coverage changes

…ptions caused by wrong topicName (#21997) Similar to: #20131 The master branch has fixed the issue by #19841 Since it will makes users can not receive the messages which created in mistake, we did not cherry-pick #19841 into other branches, see detail #19841) ### Motivation #### Background of Admin API `PersistentTopics.createSubscription` It works like this: 1. createSubscription( `tp1` ) 2. is partitioned topic? `no`: return subscriptions `yes`: createSubscription(`tp1-partition-0`)....createSubscription(`tp1-partition-n`) --- #### Background of the issue of `TopicName.getPartition(int index)` ```java String partitionedTopic = "tp1-partition-0-DLQ"; TopicName partition0 = partitionedTopic.getPartition(0);// Highlight: the partition0.toString() will be "tp1-partition-0-DLQ"(it is wrong).The correct value is "tp1-partition-0-DLQ-partition-0" ``` #### Issue Therefore, if there has a partitioned topic named `tp1-partition-0-DLQ`, the method `PersistentTopics.createSubscription` will works like this: 1. call Admin API ``PersistentTopics.createSubscription("tp1-partition-0-DLQ")` 2. is partitioned topic? 3. yes, call `TopicName.getPartition(0)` to get partition 0 and will get `tp1-partition-0-DLQ` , then loop to step-1. Then the infinite HTTP call `PersistentTopics.createSubscription` makes the broker crash. ### Modifications #### Quick fix(this PR does it) If hits the issue which makes the topic name wrong, do not loop to step 1. #### Long-term fix The PR #19841 fixes the issue which makes the topic name wrong, and this PR will create unfriendly compatibility, and PIP 263 #20033 will make compatibility good.

…ptions caused by wrong topicName (apache#21997) Similar to: apache#20131 The master branch has fixed the issue by apache#19841 Since it will makes users can not receive the messages which created in mistake, we did not cherry-pick apache#19841 into other branches, see detail apache#19841) It works like this: 1. createSubscription( `tp1` ) 2. is partitioned topic? `no`: return subscriptions `yes`: createSubscription(`tp1-partition-0`)....createSubscription(`tp1-partition-n`) --- ```java String partitionedTopic = "tp1-partition-0-DLQ"; TopicName partition0 = partitionedTopic.getPartition(0);// Highlight: the partition0.toString() will be "tp1-partition-0-DLQ"(it is wrong).The correct value is "tp1-partition-0-DLQ-partition-0" ``` Therefore, if there has a partitioned topic named `tp1-partition-0-DLQ`, the method `PersistentTopics.createSubscription` will works like this: 1. call Admin API ``PersistentTopics.createSubscription("tp1-partition-0-DLQ")` 2. is partitioned topic? 3. yes, call `TopicName.getPartition(0)` to get partition 0 and will get `tp1-partition-0-DLQ` , then loop to step-1. Then the infinite HTTP call `PersistentTopics.createSubscription` makes the broker crash. If hits the issue which makes the topic name wrong, do not loop to step 1. The PR apache#19841 fixes the issue which makes the topic name wrong, and this PR will create unfriendly compatibility, and PIP 263 apache#20033 will make compatibility good. (cherry picked from commit 4386401)

lhotari · 2024-07-29T09:34:33Z

@poorbarcode The test PartitionKeywordCompatibilityTest is broken in branch-3.0 . Cherry-picking this PR fixes the issue. Please let me know if you have concerns about cherry-picking this PR to branch-3.0.

poorbarcode · 2024-07-29T09:48:34Z

@poorbarcode The test PartitionKeywordCompatibilityTest is broken in branch-3.0 . Cherry-picking this PR fixes the issue. Please let me know if you have concerns about cherry-picking this PR to branch-3.0.

https://github.com/apache/pulsar/pull/19841/files#diff-93feae220f18ea80cb01ec7f2cefeab410aa0f3f181eed1206da4a294db0f701R233-R234 This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed

lhotari · 2024-07-29T10:00:48Z

This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed

@poorbarcode In which case would this apply?

poorbarcode · 2024-07-29T10:05:11Z

@lhotari

This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed

@poorbarcode In which case would this apply?

See the section 4 in the Motivation

lhotari · 2024-07-29T10:25:29Z

@lhotari

This changed the behavior of getPartition(index) , which will choose different topic than before, it may cause the data in the original topic can not be consumed

@poorbarcode In which case would this apply?

See the section 4 in the Motivation

@poorbarcode Since #22705 has been cherry-picked to branch-3.0, does it prevent the problem from occurring?

…ndex) (#19841) (cherry picked from commit 6e585ac)

poorbarcode self-assigned this Mar 16, 2023

poorbarcode added the release/2.9.6 label Mar 16, 2023

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

poorbarcode added release/2.10.5 release/2.11.1 type/bug The PR fixed a bug or issue reported a bug area/broker labels Mar 16, 2023

poorbarcode force-pushed the fix/infinite_get_subscriptions branch from 8cdcb15 to 8a7d584 Compare March 20, 2023 03:08

Technoboy- added this to the 3.0.0 milestone Mar 20, 2023

Technoboy- added the ready-to-test label Mar 20, 2023

Technoboy- closed this Mar 20, 2023

Technoboy- reopened this Mar 20, 2023

Technoboy- approved these changes Mar 20, 2023

View reviewed changes

codelipenghui added the release/important-notice The changes which are important should be mentioned in the release note label Mar 28, 2023

Technoboy- changed the title ~~[fix] [broker] fix infinite HTTP call getSubscriptions caused by wrong topicName~~ [fix] [broker] Fix infinite HTTP call getSubscriptions caused by wrong topicName Apr 6, 2023

poorbarcode mentioned this pull request Apr 6, 2023

PIP-263: Just auto-create no-partitioned DLQ And Prevent auto-create a DLQ for a DLQ #20033

Closed

sijie mentioned this pull request Apr 6, 2023

ISSUE-20033: PIP-263: Just auto-create no-partitioned DLQ And Prevent auto-create a DLQ for a DLQ streamnative/pulsar-archived#5695

Open

Technoboy- removed release/2.11.1 release/2.10.5 release/2.9.6 labels Apr 7, 2023

codelipenghui reviewed Apr 7, 2023

View reviewed changes

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

poorbarcode mentioned this pull request Apr 18, 2023

[fix] [broker] Fast fix infinite HTTP call getSubscriptions caused by wrong topicName #20131

Merged

4 tasks

poorbarcode added 2 commits April 23, 2023 10:50

rewrite test

3cf0dd9

fix test

70d98a9

poorbarcode added 3 commits April 23, 2023 10:51

add another case to reproduce the issue

d3ec723

fix test

8f75607

delete test TopicNameForInfiniteHttpCallGetSubscriptionsTest

73cb806

poorbarcode force-pushed the fix/infinite_get_subscriptions branch from e6f54b6 to 73cb806 Compare April 23, 2023 02:55

poorbarcode changed the title ~~[fix] [broker] Fix infinite HTTP call getSubscriptions caused by wrong topicName~~ [fix] [broker] Fix wrong logic of method TopicName.getPartition(int index) Apr 23, 2023

Technoboy- merged commit 6e585ac into apache:master Apr 23, 2023

poorbarcode deleted the fix/infinite_get_subscriptions branch April 23, 2023 09:56

poorbarcode added release/3.0.1 and removed release/3.0.1 labels May 7, 2023

poorbarcode mentioned this pull request May 31, 2023

[feat][doc] PIP-263: Just auto-create no-partitioned DLQ And Prevent auto-create a DLQ for a DLQ #20445

Open

4 tasks

poorbarcode mentioned this pull request Jan 30, 2024

[fix] [broker] [branch-3.0] Fast fix infinite HTTP call createSubscriptions caused by wrong topicName #21997

Merged

4 tasks

poorbarcode mentioned this pull request May 14, 2024

[improve] [broker] [break change] Do not create partitioned DLQ/Retry topic automatically #22705

Merged

4 tasks

lhotari added the release/3.0.6 label Jul 29, 2024

lhotari pushed a commit that referenced this pull request Jul 29, 2024

[fix] [broker] Fix wrong logic of method TopicName.getPartition(int i…

94bcf77

…ndex) (#19841) (cherry picked from commit 6e585ac)

lhotari added the cherry-picked/branch-3.0 label Jul 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] [broker] Fix wrong logic of method TopicName.getPartition(int index) #19841

[fix] [broker] Fix wrong logic of method TopicName.getPartition(int index) #19841

poorbarcode commented Mar 16, 2023 •

edited

Loading

wangjialing218 commented Mar 21, 2023

poorbarcode commented Mar 21, 2023

poorbarcode commented Mar 22, 2023

codelipenghui left a comment

codelipenghui Apr 7, 2023

poorbarcode Apr 7, 2023

poorbarcode Apr 7, 2023 •

edited

Loading

poorbarcode commented Apr 18, 2023

codecov-commenter commented Apr 23, 2023

lhotari commented Jul 29, 2024

poorbarcode commented Jul 29, 2024

lhotari commented Jul 29, 2024

poorbarcode commented Jul 29, 2024

lhotari commented Jul 29, 2024 •

edited

Loading

[fix] [broker] Fix wrong logic of method TopicName.getPartition(int index) #19841

[fix] [broker] Fix wrong logic of method TopicName.getPartition(int index) #19841

Conversation

poorbarcode commented Mar 16, 2023 • edited Loading

Motivation

Modifications

(Highlight) Note: New versions containing this patch cannot be upgraded smoothly

1. If users use the feature DLQ and enable topic auto-creation, the DLQ will be auto-created. For example:

2. And if users set the rule of the topic auto-creation like this:

3. (Highlight)And if hit current issue:

4. What happens when you hit the issue and upgrade to the new version

5. Workaround

6. (Highlight) Whether need a multi-partition DLQ/Retry Topic

Documentation

Matching PR in forked repository

wangjialing218 commented Mar 21, 2023

poorbarcode commented Mar 21, 2023

poorbarcode commented Mar 22, 2023

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui Apr 7, 2023

Choose a reason for hiding this comment

poorbarcode Apr 7, 2023

Choose a reason for hiding this comment

poorbarcode Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

poorbarcode commented Apr 18, 2023

codecov-commenter commented Apr 23, 2023

Codecov Report

lhotari commented Jul 29, 2024

poorbarcode commented Jul 29, 2024

lhotari commented Jul 29, 2024

poorbarcode commented Jul 29, 2024

lhotari commented Jul 29, 2024 • edited Loading

poorbarcode commented Mar 16, 2023 •

edited

Loading

poorbarcode Apr 7, 2023 •

edited

Loading

lhotari commented Jul 29, 2024 •

edited

Loading