Skip to content

[fix][broker]Fix removing replica clusters on topic-policies not take effect - #16144

Merged
codelipenghui merged 2 commits into
apache:masterfrom
AnonHxy:fix_topic_replic_cluster_remove
Jul 4, 2022
Merged

[fix][broker]Fix removing replica clusters on topic-policies not take effect#16144
codelipenghui merged 2 commits into
apache:masterfrom
AnonHxy:fix_topic_replic_cluster_remove

Conversation

@AnonHxy

@AnonHxy AnonHxy commented Jun 20, 2022

Copy link
Copy Markdown
Contributor

Motivation

  • Fix org.apache.pulsar.client.admin.Topics#removeReplicationClusters not taking effect: Because we have used HierarchyTopicPolicies to save topic-level policies, so we shoud removing policies.getReplicationClusters() != null.

Modifications

  • Removing policies.getReplicationClusters() != null when topic-level policies updating
  • Also, we need initialize namespace-policy first and then initialize topic-policy when PersistentTopic#initialize. The reason is that:
    • namespace-policy always has local cluster name in Policies#replication_clusters when using adminV2.
    • topic-policy may not contains local cluster name in TopicPolicies#replicationClusters.
    • If we follow the steps listed below, the persistent topic will be deleted unexpectedly, which makes the TopicPoliciesTest#testGetSetSubscribeRate test failure.
      1. admin.topics().createPartitionedTopic, which just create zookeeper nodes without PersistentTopic creating
      2. admin.namespaces().setSubscribeRate, namespace-policy will be set with local cluster name in Policies#replication_clusters. But because step.a just create zookeeper nodes, so the policy will not be applied to PersistentTopic now.
      3. admin.topicPolicies().setSubscribeRate, topic-policy will be set with no local cluster name in TopicPolicies#replicationClusters.
      4. pulsarClient1.newConsumer().subscriptionName("sub1").topic(persistenceTopic).consumerName("test").subscribe(), will create PersistentTopic
      5. if we initialize topic-policy first, the HierarchyTopicPolicies#replicationClusters will be empty , and the PersistentTopic will be deleted unexpectedly. line:1377
        public CompletableFuture<Void> checkReplication() {
        TopicName name = TopicName.get(topic);
        if (!name.isGlobal() || NamespaceService.isHeartbeatNamespace(name)) {
        return CompletableFuture.completedFuture(null);
        }
        if (log.isDebugEnabled()) {
        log.debug("[{}] Checking replication status", name);
        }
        List<String> configuredClusters = topicPolicies.getReplicationClusters().get();
        int newMessageTTLinSeconds = topicPolicies.getMessageTTLInSeconds().get();
        String localCluster = brokerService.pulsar().getConfiguration().getClusterName();
        // if local cluster is removed from global namespace cluster-list : then delete topic forcefully
        // because pulsar doesn't serve global topic without local repl-cluster configured.
        if (TopicName.get(topic).isGlobal() && !configuredClusters.contains(localCluster)) {
        log.info("Deleting topic [{}] because local cluster is not part of "
        + " global namespace repl list {}", topic, configuredClusters);
        return deleteForcefully();
        }

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

(example:)

  • *Added UT org.apache.pulsar.broker.service.ReplicatorTopicPoliciesTest#testRemoveReplicationClusters

Documentation

  • doc-not-needed

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jun 20, 2022

@Jason918 Jason918 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

@Jason918

Copy link
Copy Markdown
Contributor

/pulsarbot run-failure-checks

@AnonHxy AnonHxy changed the title [fix][broker]Fix removing replica clusters on topic-policies not take effect [WIP][fix][broker]Fix removing replica clusters on topic-policies not take effect Jun 20, 2022
@AnonHxy AnonHxy changed the title [WIP][fix][broker]Fix removing replica clusters on topic-policies not take effect [fix][broker]Fix removing replica clusters on topic-policies not take effect Jun 21, 2022
@AnonHxy

AnonHxy commented Jun 21, 2022

Copy link
Copy Markdown
Contributor Author

Also modify the namespace-policy and topic-policy initialization order. PTAL @Jason918

@AnonHxy

AnonHxy commented Jun 21, 2022

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@AnonHxy

AnonHxy commented Jun 24, 2022

Copy link
Copy Markdown
Contributor Author

@Technoboy- @codelipenghui PTAL

@AnonHxy

AnonHxy commented Jul 1, 2022

Copy link
Copy Markdown
Contributor Author

@gaozhangmin PTAL

@codelipenghui
codelipenghui merged commit 4a6622e into apache:master Jul 4, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Jul 5, 2022
wuxuanqicn pushed a commit to wuxuanqicn/pulsar that referenced this pull request Jul 14, 2022
lhotari added a commit to lhotari/pulsar that referenced this pull request Jul 2, 2026
…-persistent topics

PR apache#16144 added initTopicPolicy for PersistentTopic but not for non-persistent topics: a
NonPersistentTopic registered its policy listener in the constructor and completed the listener wrapper
with null policies, so it never applied its own topic-level policies on load and instead depended on the
namespace-wide policy broadcast.

Move the topicPolicyListener wrapper field and initTopicPolicy() up to AbstractTopic
(getTopicPolicyListener() now returns the wrapper) and have NonPersistentTopic.initialize() call
initTopicPolicy(), so a non-persistent topic registers its listener and applies its initial global and
local policies on load, exactly like a persistent topic. This removes the need to re-broadcast every
topic's policy when a namespace's policy cache finishes loading (handled in a follow-up commit).

Assisted-by: Claude Code (Opus 4.8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants