-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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] Fix namespace bundle stuck in unloading status #21445
[fix][broker] Fix namespace bundle stuck in unloading status #21445
Conversation
pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceUnloadingTest.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Outdated
Show resolved
Hide resolved
}); | ||
} | ||
return loadOrCreatePersistentTopic(tpName, createIfMissing, properties); | ||
final CompletableFuture<Optional<TopicPolicies>> topicPoliciesFuture = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test to cover the getTopic
method and the TopicPolicies
are loaded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good.
Just left a few minor comments.
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicAutoCreationTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceUnloadingTest.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/namespace/NamespaceUnloadingTest.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #21445 +/- ##
============================================
+ Coverage 73.23% 73.24% +0.01%
+ Complexity 32674 32648 -26
============================================
Files 1892 1892
Lines 140632 140645 +13
Branches 15467 15467
============================================
+ Hits 102986 103010 +24
+ Misses 29565 29519 -46
- Partials 8081 8116 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
|
PR #21231 made user topic creation rely on system topic `__change_event` if the user is enabling `topicLevelPoliciesEnabled`. It will introduce a race condition with namespace bundle unloading. All creating topics want to create `__change_event` reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout. - Get the topic policy before loading.
…21445) ### Motivation PR apache#21231 made user topic creation rely on system topic `__change_event` if the user is enabling `topicLevelPoliciesEnabled`. It will introduce a race condition with namespace bundle unloading. All creating topics want to create `__change_event` reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout. ### Modifications - Get the topic policy before loading.
…21445) (apache#21565) Cherry-pick apache#21445 to branch-3.0
…21445) (apache#21567) Co-authored-by: Jiwe Guo <technoboy@apache.org>
…21445) PR apache#21231 made user topic creation rely on system topic `__change_event` if the user is enabling `topicLevelPoliciesEnabled`. It will introduce a race condition with namespace bundle unloading. All creating topics want to create `__change_event` reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout. - Get the topic policy before loading.
…21445) PR apache#21231 made user topic creation rely on system topic `__change_event` if the user is enabling `topicLevelPoliciesEnabled`. It will introduce a race condition with namespace bundle unloading. All creating topics want to create `__change_event` reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout. - Get the topic policy before loading.
…licies before loading topics ### Motivation apache#21445 introduces an implicit requirement that if the topic policies cannot be loaded, `BrokerService#getTopic` will fail and then the client will retry loading the topic. It could break some existing usages like the tests in the C++ client: apache/pulsar-client-cpp#394 This change is applied only to avoid the race condition when unloading a namespace bundle. However, the client should not fail if the topic-level policies are not available. ### Modifications If the topic policies cannot be loaded due to a non-retryable error, we should not fail the `getTopic`. For retryable errors, the client will still retry until the broker gets the topic-level policies successfully after some attempts. Modify `TokenAuthenticatedProducerConsumerTest` to protect the change.
Motivation
PR #21231 made user topic creation rely on system topic
__change_event
if the user is enablingtopicLevelPoliciesEnabled
.It will introduce a race condition with namespace bundle unloading. All creating topics want to create
__change_event
reader but constantly fail by namespace-bundle inactive and retry mechanism. Unfortunately, the namespace-bundle unloading operation is waiting for all the topics to be completed and then release inactive status. Therefore, they will be stuck in a deadlock until one gets a timeout.Modifications
Verifying this change
Documentation
doc
doc-required
doc-not-needed
doc-complete