You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/11/13 22:30:10 UTC
[GitHub] [pulsar] rdhabalia opened a new pull request #8564: [pulsar-broker] Fix topic policy update
rdhabalia opened a new pull request #8564:
URL: https://github.com/apache/pulsar/pull/8564
### 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #8564: [pulsar-broker] Fix topic policy update
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #8564:
URL: https://github.com/apache/pulsar/pull/8564#issuecomment-727139620
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap closed pull request #8564: [pulsar-broker] Fix topic policy update
Posted by GitBox <gi...@apache.org>.
zymap closed pull request #8564:
URL: https://github.com/apache/pulsar/pull/8564
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #8564: [pulsar-broker] Fix topic policy update
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #8564:
URL: https://github.com/apache/pulsar/pull/8564#discussion_r523390675
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
##########
@@ -102,6 +102,8 @@
@Override
public void setup() throws Exception {
conf.setLoadBalancerEnabled(true);
+ conf.setTopicLevelPoliciesEnabled(true);
+ conf.setSystemTopicEnabled(true);
Review comment:
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8564: [pulsar-broker] Fix topic policy update
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8564:
URL: https://github.com/apache/pulsar/pull/8564#discussion_r526556808
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -1455,6 +1455,7 @@ public void getDeduplicationEnabled(@Suspended final AsyncResponse asyncResponse
@PathParam("namespace") String namespace,
@PathParam("topic") @Encoded String encodedTopic) {
validateTopicName(tenant, namespace, encodedTopic);
+ validateTopicOwnership(topicName, true);
Review comment:
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8564: [pulsar-broker] Fix topic policy update
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8564:
URL: https://github.com/apache/pulsar/pull/8564#discussion_r524872949
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
##########
@@ -1343,6 +1345,20 @@ public void testInvalidBundleErrorResponse() throws Exception {
}
}
+ @Test
+ public void testDedupTopicOwnership() throws Exception {
Review comment:
Seems the tests can't cover the changes? I think we must start more than 1 brokers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #8564: [pulsar-broker] Fix topic policy update
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #8564:
URL: https://github.com/apache/pulsar/pull/8564#issuecomment-785513934
Because this PR is opened for a long time. I will close this PR. Feel free to reopen it if you needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org