Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][broker] Avoid execute prepareInitPoliciesCacheAsync if namespace is deleted #22268

Merged
merged 6 commits into from
Mar 17, 2024

Conversation

hanmz
Copy link
Contributor

@hanmz hanmz commented Mar 14, 2024

Motivation

If the namespace has been deleted, prepareInitPoliciesCacheAsync should not be executed.

More detailed description:
When the namespace is forcibly deleted, if a topic exists and the own bundle of the topic is not loaded into the memory, deleting the namespace will trigger the loading of the bundle, which will trigger the execution of prepareInitPoliciesCacheAsync(load topic policies). However, at this time, due to the namespace is already in a deleted state, so execution will print the following exception log.

Minimal reproduce step:
1.Start a single broker node and create a tenant test-tenant

2.Create a new namespace and a new partitioned topic.
sh bin/pulsar-admin namespaces create test-tenant/test-ns
sh bin/pulsar-admin topics create-partitioned-topic test-tenant/test-ns/test-topic -p 1

3.Delete namespace force.
sh bin/pulsar-admin namespaces delete -f test-tenant/test-ns

We will see the following log:
image

Modifications

Skip prepare init policies cache if the namespace has been deleted.

Documentation

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

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 14, 2024
@nodece nodece requested a review from Technoboy- March 15, 2024 07:34
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 73.64%. Comparing base (bbc6224) to head (b95bc37).
Report is 60 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22268      +/-   ##
============================================
+ Coverage     73.57%   73.64%   +0.07%     
+ Complexity    32624    32234     -390     
============================================
  Files          1877     1887      +10     
  Lines        139502   139396     -106     
  Branches      15299    15287      -12     
============================================
+ Hits         102638   102664      +26     
+ Misses        28908    28785     -123     
+ Partials       7956     7947       -9     
Flag Coverage Δ
inttests 26.92% <70.83%> (+2.34%) ⬆️
systests 24.45% <62.50%> (+0.13%) ⬆️
unittests 72.91% <91.66%> (+0.07%) ⬆️

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

Files Coverage Δ
.../service/SystemTopicBasedTopicPoliciesService.java 75.33% <91.66%> (+1.13%) ⬆️

... and 171 files with indirect coverage changes

@dao-jun dao-jun merged commit 96d77f7 into apache:master Mar 17, 2024
51 checks passed
@Technoboy-
Copy link
Contributor

@dao-jun Could you set the related milestone and labels before merging?

@Technoboy-
Copy link
Contributor

Technoboy- commented Mar 18, 2024

@nodece @poorbarcode Do you check the test codes whether it is matched with the reproduced steps ?
I think the test can pass without this change @hanmz

@dao-jun
Copy link
Member

dao-jun commented Mar 18, 2024

@dao-jun Could you set the related milestone and labels before merging?

oh, I didn't noticed

@poorbarcode
Copy link
Contributor

poorbarcode commented Mar 18, 2024

Do you check the test codes whether it is matched with the reproduced steps ?
I think the test can pass without this change @hanmz

The test could not pass. The reproduce steps is below:

  • create a namespace.
  • set the ns.policies.deleted to true before the system topic __change_events was initialized.
  • try to create a new user topic; it will fail and get an error log __change_events does not exist

But the fix will cause another issue in the following steps:

  • Create a namespace.
  • Create a new user topic before the system topic __change_events is initialized.
  • The new topic will be initialized, and the topic-level policies will be lost, which lead the topic to run with the wrong policies.

BTW, the new issue will also happen when the internal reader starts failing(before the current PR). See https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java#L351-L361

@nodece
Copy link
Member

nodece commented Mar 18, 2024

This test works fine, but assertNull(service.getPoliciesCacheInit(namespaceName)); should be added after service.prepareInitPoliciesCacheAsync(namespaceName).get();.

@Technoboy-
Copy link
Contributor

This test works fine, but assertNull(service.getPoliciesCacheInit(namespaceName)); should be added after service.prepareInitPoliciesCacheAsync(namespaceName).get();.

yes, I have tested locally, it's fine.

lhotari pushed a commit that referenced this pull request Mar 27, 2024
lhotari pushed a commit that referenced this pull request Mar 27, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 15, 2024
…e is deleted (apache#22268)

(cherry picked from commit 96d77f7)
(cherry picked from commit d4610a0)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
…e is deleted (apache#22268)

(cherry picked from commit 96d77f7)
(cherry picked from commit d4610a0)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
…e is deleted (apache#22268)

(cherry picked from commit 96d77f7)
(cherry picked from commit d4610a0)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 19, 2024
…e is deleted (apache#22268)

(cherry picked from commit 96d77f7)
(cherry picked from commit d4610a0)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
…e is deleted (apache#22268)

(cherry picked from commit 96d77f7)
(cherry picked from commit d4610a0)
hanmz added a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants