Skip to content

[pulsar-broker] Fix topic policy update#8564

Closed
rdhabalia wants to merge 4 commits intoapache:masterfrom
rdhabalia:dedup_refresh
Closed

[pulsar-broker] Fix topic policy update#8564
rdhabalia wants to merge 4 commits intoapache:masterfrom
rdhabalia:dedup_refresh

Conversation

@rdhabalia
Copy link
Contributor

Motivation

Right now, TopicPolicy doesn't have auto refresh mechanism and trigger of refresh is admin api change. Now, enabling dedup admin api call can go to any broker and owner broker doesn't refresh and apply the change. Also, listening to all topics for every broker might not be scalable so, let redirect to broker which owns the topic and update the topic policy cache.

Modification

redirect and let topic owner broker to enable dedup and refresh the topic policy cache.

@rdhabalia rdhabalia added this to the 2.7.0 milestone Nov 13, 2020
@rdhabalia rdhabalia self-assigned this Nov 13, 2020
@rdhabalia
Copy link
Contributor Author

/pulsarbot run-failure-checks

public void setup() throws Exception {
conf.setLoadBalancerEnabled(true);
conf.setTopicLevelPoliciesEnabled(true);
conf.setSystemTopicEnabled(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is affecting all of the other tests in this file.
Do we have some side effect?
Are we going to use more and more resources for al the tests in this file?
My question is more general, sometimes we enable feature in common setup utilities, with the possibility to alter the semantics of the other tests.

Probably in this case there is no issue. Please clarify, I am still new to this suite of tests

}

@Test
public void testDedupTopicOwnership() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the tests can't cover the changes? I think we must start more than 1 brokers.

@PathParam("namespace") String namespace,
@PathParam("topic") @Encoded String encodedTopic) {
validateTopicName(tenant, namespace, encodedTopic);
validateTopicOwnership(topicName, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to validate the topic owner ship for topic level policy since after the policy updated, the broker will get a policy change event, if the broker own the topic, the broker will apply the policy change to that topic

@zymap
Copy link
Member

zymap commented Feb 25, 2021

Because this PR is opened for a long time. I will close this PR. Feel free to reopen it if you needed.

@zymap zymap closed this Feb 25, 2021
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.

4 participants

Comments