Skip to content
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

[ISSUE 7758]Support set Max Producer on topic level #7839

Closed
wants to merge 8 commits into from
Closed

[ISSUE 7758]Support set Max Producer on topic level #7839

wants to merge 8 commits into from

Conversation

zhanghaou
Copy link
Contributor

Link #7758 and master issue #2688

Motivation

Support set/get/remove maxProducers on a topic level.

Verifying this change

new unit test added.

@zhanghaou
Copy link
Contributor Author

/pulsarbot run-failure-checks

log.info("MaxProducers: {} get on topic: {}", getMaxProducers, testTopic);
Assert.assertEquals(getMaxProducers, maxProducers);

admin.topics().removeMaxProducers(testTopic);
Copy link
Member

Choose a reason for hiding this comment

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

How about change this test into this logic:

  • maxProducers = 2;
  • after set, can only create 2 producers;
  • after remove, can create more than 2 producers;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I will change it.

@codelipenghui codelipenghui added this to the 2.7.0 milestone Aug 21, 2020
@codelipenghui codelipenghui added component/topic-policy doc-required Your PR changes impact docs and you will update later. labels Aug 21, 2020
@@ -337,4 +338,80 @@ public void testRemovePersistence() throws Exception {

admin.topics().deletePartitionedTopic(testTopic, true);
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should also consider add test along with policies in namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

@@ -2352,6 +2352,50 @@ protected void internalGetPersistence(AsyncResponse asyncResponse){
return pulsar().getTopicPoliciesService().updateTopicPoliciesAsync(topicName, topicPolicies.get());
}

protected void internalGetMaxProducers(AsyncResponse asyncResponse){
validateAdminAccessForTenant(namespaceName.getTenant());
Copy link
Contributor

Choose a reason for hiding this comment

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

Call checkTopicLevelPolicyEnable() before use topic policy API, please check all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

@@ -2352,6 +2352,50 @@ protected void internalGetPersistence(AsyncResponse asyncResponse){
return pulsar().getTopicPoliciesService().updateTopicPoliciesAsync(topicName, topicPolicies.get());
}

protected void internalGetMaxProducers(AsyncResponse asyncResponse){
Copy link
Member

Choose a reason for hiding this comment

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

How about make the method name more clear and align with namespace policies?
e.g. add "PerTopic" at the end: "xxxMaxProducersPerTopic"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it is the policies for a single topic, namespaces policies can take control of all the topics of the namespace, so 'perTopic' is suitable, but at the topic level, just set to only one topic, so I remove the 'perTopic' in all methods.

Copy link
Member

@jiazhai jiazhai left a comment

Choose a reason for hiding this comment

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

Thanks @zhanghaou for the help, overall lgtm. left some comments related to the test and method names.

zhanghaou and others added 3 commits August 22, 2020 08:48
…ic-level

# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
#	pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
@zhanghaou
Copy link
Contributor Author

/pulsarbot run-failure-checks

4 similar comments
@zhanghaou
Copy link
Contributor Author

/pulsarbot run-failure-checks

@zhanghaou
Copy link
Contributor Author

/pulsarbot run-failure-checks

@zhanghaou
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie
Copy link
Member

sijie commented Aug 24, 2020

/pulsarbot run-failure-checks

@zhanghaou zhanghaou closed this Aug 27, 2020
@Anonymitaet Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants