[fix][broker] Fix the issue of topics possibly being deleted. #21704

crossoverJie · 2023-12-10T10:19:36Z

Fixes #21653

Motivation

Reproduce process:
#21653 (comment)

Modifications

The registerTopicPolicyListener() function was moved from PersistentTopic() to initTopicPolicy().

Flow diagram:

Current:

After:

Verifying this change

Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

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

Matching PR in forked repository

PR in forked repository: crossoverJie#19

mattisonchao · 2023-12-11T02:12:55Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

@@ -1632,6 +1631,11 @@ public CompletableFuture<Void> checkReplication() {
        }

        List<String> configuredClusters = topicPolicies.getReplicationClusters().get();
+        if (CollectionUtils.isEmpty(configuredClusters)) {
+            log.warn("[{}] No replication clusters configured", name);


It should be an abnormal situation, please add some explanation here to indicate what happens here. :)

In our production env, because configuredClusters is empty, it caused the Topic to be deleted.

Referring to the above flow diagram, method onUpdate()->checkReplication() is called before method this.updateTopicPolicyByNamespacePolicy(policies).

Yes, maybe we can refine the log to let other users know about the possibility of a problem. :)

mattisonchao

LGTM, Is it possible to add a test? Maybe running it many times can reproduce it, which is also acceptable. :)

crossoverJie · 2023-12-11T02:41:26Z

Maybe running it many times can reproduce it

#21653 (comment)

Its difficult to reproduce with unit tests, manually thread.sleep() can reproduce it😂.

codecov-commenter · 2023-12-11T08:13:55Z

Codecov Report

Merging #21704 (ca2d28c) into master (495b141) will increase coverage by 36.62%.
Report is 13 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #21704       +/-   ##
=============================================
+ Coverage     36.75%   73.37%   +36.62%     
- Complexity    12271    32767    +20496     
=============================================
  Files          1717     1893      +176     
  Lines        131197   140761     +9564     
  Branches      14339    15509     +1170     
=============================================
+ Hits          48220   103290    +55070     
+ Misses        76596    29360    -47236     
- Partials       6381     8111     +1730

Flag	Coverage Δ
inttests	`24.11% <40.00%> (-0.01%)`	⬇️
systests	`24.65% <40.00%> (-0.13%)`	⬇️
unittests	`72.67% <100.00%> (+40.80%)`	⬆️

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

Files	Coverage Δ
...pulsar/broker/admin/impl/PersistentTopicsBase.java	`64.89% <ø> (+54.59%)`	⬆️
...sar/broker/service/persistent/PersistentTopic.java	`78.82% <100.00%> (+26.74%)`	⬆️

... and 1463 files with indirect coverage changes

codelipenghui · 2023-12-12T01:55:14Z

@crossoverJie, can we just put a time-consuming task in the thread pool to simulate the sleep behavior?
Or maybe you can just push your code with the sleep and we can find a solution together.

It's a very critical fix, if don't have a test to protect the change, things will go wrong again in the future. Here is an example: #21686

cc @mattisonchao @Technoboy-

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

Co-authored-by: Jiwe Guo <technoboy@apache.org> (cherry picked from commit 84ea1ca)

…#21704) Co-authored-by: Jiwe Guo <technoboy@apache.org> (cherry picked from commit 84ea1ca) (cherry picked from commit 49490b3)

poorbarcode · 2024-01-05T08:42:32Z

Sorry, to clear the root cause of the issue, I left a comment:

The `conditions-1` to encounter the issue:

You enabled replication on the namespace level and did not enable replication on the topic level.
You enabled topic level policies
The flowing events happen at the same time
- Modify topic-level policies, such as pulsar-admin topics set-dispatch-rate. Note: Modifying namespace-level policies will not trigger the issue.
- The target topic is loading now.

The `conditions-2` to encounter the issue:

You enabled replication on the namespace level and did not enable replication on the topic level.
You enabled topic-level policies
Restart brokers
Note:
- this case often happens when there is only 1 broker under your cluster,
- the probability is rare if there are more than 1 brokers under your cluster. If you restart all brokers at the same time, the probability rises as only 1 broker.

Background & Summary

checkCompaction will delete the topic if the attribute replicationClusters does not match the current cluster( in other words, the attribute replicationClusters does not contain broker.conf.cluster_name )
topic level policy modification and namespace level policy modification will trigger a task checkCompaction.
When a topic is loading, it is initialized by the following steps:
a. Registered a listener to accept the topic level policy modification event
b. Initialize the attribute replicationClusters
TopicPoliciesService will read all topic policies and try to update topic-level policies for every topic under this broker after the Broker restart.

After section 3-a, the task checkCompaction may be triggered by a topic-level policy modification(see section 3), but at this time the replicationClustershas not been initialized. It causes thesection 2` to delete the topic.

Detail steps of the issue: `conditions-1`

time	task: `topic loading`	task: `modifying topic policies`
1	Registered the topic to accept new changes of Topic Policies; the attribute `replicationClusters` is empty now
2		Modify topic policies
3		Trigger a task `checkReplication`, and then it will try to delete the topic due to `replicationClusters` being empty
4	Initialize the attribute `replicationClusters` from Namespace level Replication settings

Detail steps of the issue: `conditions-2`

time	task: `topic loading`	task: `TopicPoliciesService initialize after brokers restart`
1	Registered the topic to accept new changes of Topic Policies; the attribute `replicationClusters` is empty now
2		Read all topic policies and try to update topic-level policies for every topic under this broker after the Broker restart
3		Trigger a task `checkReplication`, and then it will try to delete the topic due to `replicationClusters` being empty
4	Initialize the attribute `replicationClusters` from Namespace level Replication settings

When there is more than one broker under your cluster, the probability of shedding topics to a new broker(when it has just started right now) is low, so the probability is high if there is only 1 broker.

Workaround-1

Before you upgrade to the new version(which contains this fix), disable the feature topicLevelPoliciesEnabled, but it also makes all topic-level policies invalidate you set before.

Workaround-2

Before you upgrade to the new version(which contains this fix), do not restart brokers or modify any topic-level policies.

crossoverJie · 2024-01-05T08:52:15Z

@poorbarcode
Thanks for your clarification.

But based on the background of Workaround-2, if the current cluster has the conditions to trigger this issue, you can refer to the operation here to upgrade; otherwise, it may still cause the topic to be deleted during the upgrade(or restart) process.

poorbarcode · 2024-01-05T08:56:17Z

if the current cluster has the conditions to trigger this issue, you can refer to the #21653 (comment) to upgrade; otherwise, it may still cause the topic to be deleted during the upgrade(or restart) process.

Ah, I see. Pulsar reads all topic policies and calls topic.onUpdate for every topic when it starts. @crossoverJie Thanks ❤️

@crossoverJie I updated the comment above, please review it, thanks

crossoverJie · 2024-01-05T09:42:31Z

@poorbarcode Great job, thanks for your supplement👍.

…#21704) Co-authored-by: Jiwe Guo <technoboy@apache.org> (cherry picked from commit 84ea1ca) (cherry picked from commit 49490b3)

…#21704) Co-authored-by: Jiwe Guo <technoboy@apache.org> (cherry picked from commit 84ea1ca)

fix 21653

b2674d1

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

Technoboy- assigned crossoverJie Dec 11, 2023

Technoboy- added this to the 3.2.0 milestone Dec 11, 2023

Technoboy- added release/2.10.6 release/3.0.3 release/2.11.4 release/3.1.3 labels Dec 11, 2023

mattisonchao reviewed Dec 11, 2023

View reviewed changes

mattisonchao approved these changes Dec 11, 2023

View reviewed changes

Technoboy- added the ready-to-test label Dec 11, 2023

Technoboy- approved these changes Dec 11, 2023

View reviewed changes

Merge branch 'master' into issue-21653

c39bf5c

liangyepianzhou reviewed Dec 12, 2023

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java Outdated Show resolved Hide resolved

liangyepianzhou approved these changes Dec 12, 2023

View reviewed changes

Technoboy- and others added 2 commits December 14, 2023 16:24

add test

35055e9

Merge remote-tracking branch 'origin/issue-21653' into issue-21653

ca2d28c

codelipenghui approved these changes Dec 14, 2023

View reviewed changes

Technoboy- merged commit 84ea1ca into apache:master Dec 14, 2023
48 checks passed

poorbarcode pushed a commit that referenced this pull request Dec 26, 2023

[fix][broker] Fix the issue of topics possibly being deleted. (#21704)

49490b3

Co-authored-by: Jiwe Guo <technoboy@apache.org> (cherry picked from commit 84ea1ca)

poorbarcode pushed a commit that referenced this pull request Dec 26, 2023

[fix][broker] Fix the issue of topics possibly being deleted. (#21704)

b952f6f

Co-authored-by: Jiwe Guo <technoboy@apache.org> (cherry picked from commit 84ea1ca)

poorbarcode added cherry-picked/branch-3.0 cherry-picked/branch-3.1 labels Dec 26, 2023

poorbarcode pushed a commit that referenced this pull request Dec 26, 2023

[fix][broker] Fix the issue of topics possibly being deleted. (#21704)

7163f32

Co-authored-by: Jiwe Guo <technoboy@apache.org> (cherry picked from commit 84ea1ca)

poorbarcode pushed a commit that referenced this pull request Dec 26, 2023

[fix][broker] Fix the issue of topics possibly being deleted. (#21704)

c1867f2

Co-authored-by: Jiwe Guo <technoboy@apache.org> (cherry picked from commit 84ea1ca)

poorbarcode added cherry-picked/branch-2.10 cherry-picked/branch-2.11 labels Dec 26, 2023

shibd added a commit that referenced this pull request Jan 8, 2024

[branch-2.11] Fix license header #21704.

cf5d7cb

nodece pushed a commit to nodece/pulsar that referenced this pull request Feb 23, 2024

[fix][broker] Fix the issue of topics possibly being deleted. (apache…

583d70e

…#21704) Co-authored-by: Jiwe Guo <technoboy@apache.org> (cherry picked from commit 84ea1ca)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Fix the issue of topics possibly being deleted. #21704

[fix][broker] Fix the issue of topics possibly being deleted. #21704

crossoverJie commented Dec 10, 2023

mattisonchao Dec 11, 2023

crossoverJie Dec 11, 2023

crossoverJie Dec 11, 2023

mattisonchao Dec 11, 2023

mattisonchao left a comment

crossoverJie commented Dec 11, 2023

codecov-commenter commented Dec 11, 2023 •

edited

codelipenghui commented Dec 12, 2023

poorbarcode commented Jan 5, 2024 •

edited

crossoverJie commented Jan 5, 2024

poorbarcode commented Jan 5, 2024 •

edited

crossoverJie commented Jan 5, 2024

[fix][broker] Fix the issue of topics possibly being deleted. #21704

[fix][broker] Fix the issue of topics possibly being deleted. #21704

Conversation

crossoverJie commented Dec 10, 2023

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

mattisonchao Dec 11, 2023

Choose a reason for hiding this comment

crossoverJie Dec 11, 2023

Choose a reason for hiding this comment

crossoverJie Dec 11, 2023

Choose a reason for hiding this comment

mattisonchao Dec 11, 2023

Choose a reason for hiding this comment

mattisonchao left a comment

Choose a reason for hiding this comment

crossoverJie commented Dec 11, 2023

codecov-commenter commented Dec 11, 2023 • edited

Codecov Report

codelipenghui commented Dec 12, 2023

poorbarcode commented Jan 5, 2024 • edited

The conditions-1 to encounter the issue:

The conditions-2 to encounter the issue:

Background & Summary

Detail steps of the issue: conditions-1

Detail steps of the issue: conditions-2

Workaround-1

Workaround-2

crossoverJie commented Jan 5, 2024

poorbarcode commented Jan 5, 2024 • edited

crossoverJie commented Jan 5, 2024

codecov-commenter commented Dec 11, 2023 •

edited

poorbarcode commented Jan 5, 2024 •

edited

The `conditions-1` to encounter the issue:

The `conditions-2` to encounter the issue:

Detail steps of the issue: `conditions-1`

Detail steps of the issue: `conditions-2`

poorbarcode commented Jan 5, 2024 •

edited