[fix][broker] partitioned __change_events topic is policy topic #20392

michaeljmarshall · 2023-05-24T22:04:26Z

Fixes #20376

Motivation

The __change_events system topic is created in each namespace. It is used for topic policies. The SystemTopicNames#isSystemTopic classifies partitioned and non-partitioned __change_events topics as system topics. The SystemTopicNames#isTopicPoliciesSystemTopic only classifies non-partitioned __change_events topics as topic policies system topics. I think this is a bug, and we need to make sure that parititioned __change_events topics are topic policies system topics.

Note: I am not sure that it is recommended to use a partitioned topic for the topic policies, but for backwards compatibility, I think we have to classify a partitioned __change_events topic as a topic policies topic. Since #10850, we test partitioned __change_events topics.

The AdminApi2Test class fails frequently with an error associated with managed ledgers not being deleted yet. That error may be because of an incomplete classification of the change events system topic name. I say this because the test fails at tenant deletion, and tenant deletion appears to rely on correct classification of the topic policies topic:

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

Lines 262 to 266 in 908d0b3

    
           if (SystemTopicNames.isTopicPoliciesSystemTopic(topic)) { 
        
               partitionedTopicPolicy.add(topic); 
        
           } else { 
        
               allPartitionedSystemTopics.add(topic); 
        
           }

Modifications

Update SystemTopicNames#isTopicPoliciesSystemTopic so that it considers a partitioned __change_events topic to be a topic policies system topic.
Update PersistentTopic so that it does not do its own classification and instead relies on the SystemTopicNames class.
Fix typo in SystemTopicNames#isSystemTopic.
Add test

Verifying this change

A new test is added.

Documentation

doc-not-needed

Matching PR in forked repository

PR in forked repository: Skipping for this minor change

…ames

315157973

LGTM

michaeljmarshall · 2023-05-25T03:56:24Z

I still think this is the right change, but it is odd that the net effect is to change when we delete the topic policy topic. See:

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

Lines 257 to 294 in 908d0b3

    
           for (String topic : allPartitionedTopics) { 
        
               if (!pulsar().getBrokerService().isSystemTopic(TopicName.get(topic))) { 
        
                   hasNonSystemTopic = true; 
        
                   allUserCreatedPartitionTopics.add(topic); 
        
               } else { 
        
                   if (SystemTopicNames.isTopicPoliciesSystemTopic(topic)) { 
        
                       partitionedTopicPolicy.add(topic); 
        
                   } else { 
        
                       allPartitionedSystemTopics.add(topic); 
        
                   } 
        
               } 
        
           } 
        
           if (!force) { 
        
               if (hasNonSystemTopic) { 
        
                   throw new RestException(Status.CONFLICT, "Cannot delete non empty namespace"); 
        
               } 
        
           } 
        
           final CompletableFuture<Void> markDeleteFuture; 
        
           if (policies != null && policies.deleted) { 
        
               markDeleteFuture = CompletableFuture.completedFuture(null); 
        
           } else { 
        
               markDeleteFuture = namespaceResources().setPoliciesAsync(namespaceName, old -> { 
        
                   old.deleted = true; 
        
                   return old; 
        
               }); 
        
           } 
        
           return markDeleteFuture.thenCompose(__ -> 
        
                           internalDeleteTopicsAsync(allUserCreatedTopics)) 
        
                   .thenCompose(ignore -> 
        
                           internalDeletePartitionedTopicsAsync(allUserCreatedPartitionTopics)) 
        
                   .thenCompose(ignore -> 
        
                           internalDeleteTopicsAsync(allSystemTopics)) 
        
                   .thenCompose(ignore -> 
        
                           internalDeletePartitionedTopicsAsync(allPartitionedSystemTopics)) 
        
                   .thenCompose(ignore -> 
        
                           internalDeleteTopicsAsync(topicPolicy)) 
        
                   .thenCompose(ignore -> 
        
                           internalDeletePartitionedTopicsAsync(partitionedTopicPolicy));

codecov-commenter · 2023-05-25T03:59:39Z

Codecov Report

Merging #20392 (f852521) into master (3f2978d) will increase coverage by 36.11%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             master   #20392       +/-   ##
=============================================
+ Coverage     36.82%   72.94%   +36.11%     
- Complexity    12064    31878    +19814     
=============================================
  Files          1687     1864      +177     
  Lines        128856   138416     +9560     
  Branches      14018    15188     +1170     
=============================================
+ Hits          47456   100968    +53512     
+ Misses        75154    29440    -45714     
- Partials       6246     8008     +1762

Flag	Coverage Δ
inttests	`24.19% <100.00%> (-0.01%)`	⬇️
systests	`25.14% <85.71%> (+0.15%)`	⬆️
unittests	`72.21% <100.00%> (+40.22%)`	⬆️

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

Impacted Files	Coverage Δ
...mon/policies/data/stats/SubscriptionStatsImpl.java	`69.56% <ø> (ø)`
...sar/common/policies/data/stats/TopicStatsImpl.java	`90.44% <ø> (+36.30%)`	⬆️
...org/apache/pulsar/broker/ServiceConfiguration.java	`99.37% <100.00%> (+1.35%)`	⬆️
...sar/broker/service/persistent/PersistentTopic.java	`79.55% <100.00%> (+24.84%)`	⬆️
.../apache/pulsar/common/naming/SystemTopicNames.java	`77.77% <100.00%> (+11.11%)`	⬆️
...he/pulsar/metadata/impl/AbstractMetadataStore.java	`85.38% <100.00%> (+32.67%)`	⬆️

... and 1428 files with indirect coverage changes

michaeljmarshall · 2023-05-25T04:21:20Z

It's possible that an important part of the fix is here:

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

Lines 1345 to 1350 in ad236ca

    
           if (!SystemTopicNames.isTopicPoliciesSystemTopic(topic) 
        
                   && brokerService.getPulsar().getConfiguration().isSystemTopicEnabled()) { 
        
               return deleteTopicPolicies(); 
        
           } else { 
        
               return CompletableFuture.completedFuture(null); 
        
           }

With this change, we won't send a notification that the __change_events-partition-x topic has been deleted.

michaeljmarshall · 2023-05-25T04:32:11Z

I still think this is the right change, but it is odd that the net effect is to change when we delete the topic policy topic. See:

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

Lines 257 to 294 in 908d0b3

for (String topic : allPartitionedTopics) {

if (!pulsar().getBrokerService().isSystemTopic(TopicName.get(topic))) {

hasNonSystemTopic = true;

allUserCreatedPartitionTopics.add(topic);

} else {

if (SystemTopicNames.isTopicPoliciesSystemTopic(topic)) {

partitionedTopicPolicy.add(topic);

} else {

allPartitionedSystemTopics.add(topic);

}

}

}

if (!force) {

if (hasNonSystemTopic) {

throw new RestException(Status.CONFLICT, "Cannot delete non empty namespace");

}

}

final CompletableFuture<Void> markDeleteFuture;

if (policies != null && policies.deleted) {

markDeleteFuture = CompletableFuture.completedFuture(null);

} else {

markDeleteFuture = namespaceResources().setPoliciesAsync(namespaceName, old -> {

old.deleted = true;

return old;

});

}

return markDeleteFuture.thenCompose(__ ->

internalDeleteTopicsAsync(allUserCreatedTopics))

.thenCompose(ignore ->

internalDeletePartitionedTopicsAsync(allUserCreatedPartitionTopics))

.thenCompose(ignore ->

internalDeleteTopicsAsync(allSystemTopics))

.thenCompose(ignore ->

internalDeletePartitionedTopicsAsync(allPartitionedSystemTopics))

.thenCompose(ignore ->

internalDeleteTopicsAsync(topicPolicy))

.thenCompose(ignore ->

internalDeletePartitionedTopicsAsync(partitionedTopicPolicy));

It looks like the order is likely the key to this bug fix. When we mis-classify the topic policies topic (as we do before this fix), we likely do the following:

Delete the topic policies topic partitions.
Delete some other topic, which triggers a DELETE notification to be sent by the broker's local TopicPolicyWriter.
That deletion event triggers a producer to connect to the change events topic.

Because we always allow system topics to be auto created, the topic is created:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

Lines 3298 to 3301 in e008de9

    
           //System topic can always be created automatically 
        
           if (pulsar.getConfiguration().isSystemTopicEnabled() && isSystemTopic(topicName)) { 
        
               return CompletableFuture.completedFuture(true); 
        
           }

Then, when we go to delete the tenant (in the flaky test), we get a failure because there is a managed ledger, and that will happen to be the one for the TopicPolicyWriter.

michaeljmarshall · 2023-05-25T04:45:48Z

Note that #17609 attempted to prevent the conditions I describe above. However, I think the key mistake may be in the logic made here:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

Lines 3291 to 3301 in e008de9

    
           private CompletableFuture<Boolean> isAllowAutoTopicCreationAsync(final TopicName topicName, 
        
                                                                            final Optional<Policies> policies) { 
        
               if (policies.isPresent() && policies.get().deleted) { 
        
                   log.info("Preventing AutoTopicCreation on a namespace that is being deleted {}", 
        
                           topicName.getNamespaceObject()); 
        
                   return CompletableFuture.completedFuture(false); 
        
               } 
        
               //System topic can always be created automatically 
        
               if (pulsar.getConfiguration().isSystemTopicEnabled() && isSystemTopic(topicName)) { 
        
                   return CompletableFuture.completedFuture(true); 
        
               }

That policies object is fetched earlier in the code as:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

Lines 3037 to 3039 in e008de9

    
           Optional<Policies> policies = 
        
                   pulsar.getPulsarResources().getNamespaceResources() 
        
                           .getPoliciesIfCached(topicName.getNamespaceObject());

It would seem that getting the policies only when cached presents a problem because the notification that the namespace is being deleted might not actually be present on the broker that receives the lookup request. Perhaps we should update that logic to get the latest version of the namespace policies object.

michaeljmarshall · 2023-05-25T04:58:42Z

Now, I am wondering if #18220 was necessary because of this bug.

lhotari

LGTM. Good work @michaeljmarshall

eolivelli

Great catch

(cherry picked from commit 9918bce)

(cherry picked from commit 9918bce) backported to branch-2.10 by renaming SystemTopicsNames to EventsTopicNames

…he#20392) (cherry picked from commit 9918bce) backported to branch-2.10 by renaming SystemTopicsNames to EventsTopicNames (cherry picked from commit 323d112)

[fix][broker] partitioned __change_events topic is policy topic

ad236ca

michaeljmarshall added type/bug The PR fixed a bug or issue reported a bug area/broker doc-not-needed Your PR changes do not impact docs ready-to-test labels May 24, 2023

michaeljmarshall added this to the 3.1.0 milestone May 24, 2023

michaeljmarshall requested review from lhotari, Technoboy- and 315157973 May 24, 2023 22:04

michaeljmarshall self-assigned this May 24, 2023

michaeljmarshall mentioned this pull request May 24, 2023

Flaky-test: AdminApi2Test.resetClusters #20376

Open

2 tasks

Merge remote-tracking branch 'upstream/master' into fix-event-topic-n…

f852521

…ames

315157973 approved these changes May 25, 2023

View reviewed changes

lhotari approved these changes May 25, 2023

View reviewed changes

lhotari requested review from poorbarcode, mattisonchao, codelipenghui and hangc0276 May 25, 2023 06:00

lhotari added release/2.10.5 release/2.11.2 release/3.0.1 labels May 25, 2023

poorbarcode approved these changes May 25, 2023

View reviewed changes

lhotari mentioned this pull request May 25, 2023

[improve][broker] Create non-partitioned system topics in auto topic creation #20397

Open

4 tasks

eolivelli approved these changes May 25, 2023

View reviewed changes

nicoloboschi approved these changes May 25, 2023

View reviewed changes

nicoloboschi merged commit 9918bce into apache:master May 25, 2023

michaeljmarshall deleted the fix-event-topic-names branch May 25, 2023 13:51

michaeljmarshall added a commit that referenced this pull request May 25, 2023

[fix][broker] partitioned __change_events topic is policy topic (#20392)

b190a8a

(cherry picked from commit 9918bce)

michaeljmarshall added a commit that referenced this pull request May 25, 2023

[fix][broker] partitioned __change_events topic is policy topic (#20392)

5f38eaf

(cherry picked from commit 9918bce)

michaeljmarshall added cherry-picked/branch-2.11 cherry-picked/branch-3.0 labels May 25, 2023

lhotari pushed a commit that referenced this pull request May 26, 2023

[fix][broker] partitioned __change_events topic is policy topic (#20392)

323d112

(cherry picked from commit 9918bce) backported to branch-2.10 by renaming SystemTopicsNames to EventsTopicNames

lhotari added the cherry-picked/branch-2.10 label May 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] partitioned __change_events topic is policy topic #20392

[fix][broker] partitioned __change_events topic is policy topic #20392

michaeljmarshall commented May 24, 2023

315157973 left a comment

michaeljmarshall commented May 25, 2023

codecov-commenter commented May 25, 2023

michaeljmarshall commented May 25, 2023

michaeljmarshall commented May 25, 2023

michaeljmarshall commented May 25, 2023

michaeljmarshall commented May 25, 2023

lhotari left a comment

eolivelli left a comment

	if (SystemTopicNames.isTopicPoliciesSystemTopic(topic)) {
	partitionedTopicPolicy.add(topic);
	} else {
	allPartitionedSystemTopics.add(topic);
	}

[fix][broker] partitioned __change_events topic is policy topic #20392

[fix][broker] partitioned __change_events topic is policy topic #20392

Conversation

michaeljmarshall commented May 24, 2023

Motivation

Modifications

Verifying this change

Documentation

Matching PR in forked repository

315157973 left a comment

Choose a reason for hiding this comment

michaeljmarshall commented May 25, 2023

codecov-commenter commented May 25, 2023

Codecov Report

michaeljmarshall commented May 25, 2023

michaeljmarshall commented May 25, 2023

michaeljmarshall commented May 25, 2023

michaeljmarshall commented May 25, 2023

lhotari left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment