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

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

Motivation

Background of Admin API `PersistentTopics.getSubscriptions`

It works like this:

getSubscriptions( tp1 )
is partitioned topic?
no: return subscriptions
yes: getSubscriptions(tp1-partition-0)....getSubscriptions(tp1-partition-n)

Background of the issue of `TopicName.getPartition(int index)`

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"

see:

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

Lines 233 to 237 in da78879

    
           public TopicName getPartition(int index) { 
        
               if (index == -1 || this.toString().contains(PARTITIONED_TOPIC_SUFFIX)) { 
        
                   return this; 
        
               } 
        
               String partitionName = this.toString() + PARTITIONED_TOPIC_SUFFIX + index;

Issue

Therefore, if there has a partitioned topic named tp1-partition-0-DLQ, the method PersistentTopics.getSubscriptions will works like this:

call Admin API ``PersistentTopics.getSubscriptions("tp1-partition-0-DLQ")`
is partitioned topic?
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.getSubscriptions 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.

Documentation

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

Matching PR in forked repository

PR in forked repository:

[fix] [broker] Fast fix infinite HTTP call getSubscriptions caused by… poorbarcode/pulsar#88

… wrong topicName

lifepuzzlefun · 2023-04-19T04:59:58Z

does tp1-partition-0-DLQ partition topic is auto create by consumer ? maybe we also need fix the auto create logic.
can we only treat topic with partition-{number} suffix as partitioned topic ? for those topic name contains partition- we only lookup the last partition-{number} (contains -> lastIndexOf)

poorbarcode · 2023-04-19T06:40:15Z

...test/java/org/apache/pulsar/client/api/TopicNameForInfiniteHttpCallGetSubscriptionsTest.java

+    }
+
+    @Test
+    public void testInfiniteHttpCallGetSubscriptions() throws Exception {


@lifepuzzlefun

does tp1-partition-0-DLQ partition topic is auto create by consumer ? maybe we also need fix the auto create logic.

Yes, it is. The PIP 263 will will try to ban it

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

can we only treat topic with partition-{number} suffix as partitioned topic ? for those topic name contains partition- we only lookup the last partition-{number} (contains -> lastIndexOf)

The PR 19841 fixes the issue which makes the topic name wrong

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

I create a comment-list to track the context easier.

codelipenghui

LGTM!

codelipenghui · 2023-04-19T10:55:43Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

                .thenCompose(__ -> pulsar().getBrokerService().deleteTopic(topicName.toString(), force));
    }

+    private boolean hitsTheIssue18149(PartitionedTopicMetadata topicMetadata) {


Suggested change

private boolean hitsTheIssue18149(PartitionedTopicMetadata topicMetadata) {

private boolean isUnexpectedTopicName(PartitionedTopicMetadata topicMetadata) {

We can add the issue link as a comment

Good suggestion. Already fixed.

codelipenghui · 2023-04-20T08:33:37Z

/pulsarbot run-failure-checks

Technoboy- · 2023-04-21T03:45:19Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+            return false;
+        }
+        if (topicMetadata.partitions <= 0){
+            return false;


Why do we need to check partitions ? because it has already check at line 1207

I feel that if another Admin API hits this issue, too, this method can be reused(ensures this method itself accurately identifies the issue).

… wrong topicName (#20131)

… wrong topicName (#20131) (cherry picked from commit 0c50866)

… wrong topicName (apache#20131) (cherry picked from commit 0c50866) (cherry picked from commit eff0a29)

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

[fix] [broker] Fast fix infinite HTTP call getSubscriptions caused by…

4208925

… wrong topicName

poorbarcode self-assigned this Apr 18, 2023

poorbarcode added release/2.10.5 release/2.11.2 release/3.0.1 labels Apr 18, 2023

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

poorbarcode added area/broker area/admin type/bug The PR fixed a bug or issue reported a bug and removed doc-not-needed Your PR changes do not impact docs labels Apr 18, 2023

poorbarcode added this to the 3.1.0 milestone Apr 18, 2023

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

poorbarcode mentioned this pull request Apr 18, 2023

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

Merged

4 tasks

poorbarcode commented Apr 19, 2023

View reviewed changes

codelipenghui approved these changes Apr 19, 2023

View reviewed changes

change method name hitsTheIssue18149 -> isUnexpectedTopicName(

b9b809a

lifepuzzlefun approved these changes Apr 20, 2023

View reviewed changes

Technoboy- reviewed Apr 21, 2023

View reviewed changes

Technoboy- approved these changes Apr 23, 2023

View reviewed changes

Technoboy- merged commit 0c50866 into apache:master Apr 23, 2023

Technoboy- pushed a commit that referenced this pull request Apr 23, 2023

[fix] [broker] Fast fix infinite HTTP call getSubscriptions caused by…

8bff666

… wrong topicName (#20131)

Technoboy- added the cherry-picked/branch-2.11 label Apr 23, 2023

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

poorbarcode added a commit that referenced this pull request May 6, 2023

[fix] [broker] Fast fix infinite HTTP call getSubscriptions caused by…

eff0a29

… wrong topicName (#20131) (cherry picked from commit 0c50866)

poorbarcode added cherry-picked/branch-2.10 cherry-picked/branch-3.0 labels May 6, 2023

poorbarcode added a commit that referenced this pull request May 7, 2023

[fix] [broker] Fast fix infinite HTTP call getSubscriptions caused by…

214a6bd

… wrong topicName (#20131) (cherry picked from commit 0c50866)

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 11, 2023

[fix] [broker] Fast fix infinite HTTP call getSubscriptions caused by…

c3aed87

… wrong topicName (apache#20131) (cherry picked from commit 0c50866) (cherry picked from commit eff0a29)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Uh oh!

poorbarcode commented Apr 18, 2023

Uh oh!

lifepuzzlefun commented Apr 19, 2023

Uh oh!

poorbarcode Apr 19, 2023 •

edited

Loading

Uh oh!

codelipenghui left a comment

Uh oh!

codelipenghui Apr 19, 2023

Uh oh!

codelipenghui Apr 19, 2023

Uh oh!

poorbarcode Apr 19, 2023

Uh oh!

codelipenghui commented Apr 20, 2023

Uh oh!

Technoboy- Apr 21, 2023

Uh oh!

poorbarcode Apr 21, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	public TopicName getPartition(int index) {
	if (index == -1 \|\| this.toString().contains(PARTITIONED_TOPIC_SUFFIX)) {
	return this;
	}
	String partitionName = this.toString() + PARTITIONED_TOPIC_SUFFIX + index;

	private boolean hitsTheIssue18149(PartitionedTopicMetadata topicMetadata) {
	private boolean isUnexpectedTopicName(PartitionedTopicMetadata topicMetadata) {

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

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

Uh oh!

Conversation

poorbarcode commented Apr 18, 2023

Motivation

Background of Admin API PersistentTopics.getSubscriptions

Background of the issue of TopicName.getPartition(int index)

Issue

Modifications

Quick fix(this PR does it)

Long-term fix

Documentation

Matching PR in forked repository

Uh oh!

lifepuzzlefun commented Apr 19, 2023

Uh oh!

poorbarcode Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codelipenghui left a comment

Choose a reason for hiding this comment

Uh oh!

codelipenghui Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

codelipenghui Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

poorbarcode Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

codelipenghui commented Apr 20, 2023

Uh oh!

Technoboy- Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

poorbarcode Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Background of Admin API `PersistentTopics.getSubscriptions`

Background of the issue of `TopicName.getPartition(int index)`

poorbarcode Apr 19, 2023 •

edited

Loading

poorbarcode Apr 21, 2023 •

edited

Loading