codelipenghui commented on code in PR #18369:
URL: https://github.com/apache/pulsar/pull/18369#discussion_r1028676663
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ReplicatorTest.java:
##########
@@ -1505,4 +1505,34 @@ public void testWhenUpdateReplicationCluster() throws
Exception {
assertTrue(topic.getReplicators().isEmpty());
});
}
+
+ @Test
+ public void testReplicatorProducerNotExceed() throws Exception {
+ log.info("--- testReplicatorProducerNotExceed ---");
+ String namespace1 = "pulsar/ns11";
+ admin1.namespaces().createNamespace(namespace1);
+ admin1.namespaces().setNamespaceReplicationClusters(namespace1,
Sets.newHashSet("r1", "r2"));
+ final TopicName dest1 = TopicName.get(
+ BrokerTestUtil.newUniqueName("persistent://" + namespace1 +
"/testReplicatorProducerNotExceed1"));
+ String namespace2 = "pulsar/ns22";
+ admin2.namespaces().createNamespace(namespace2);
+ admin2.namespaces().setNamespaceReplicationClusters(namespace2,
Sets.newHashSet("r1", "r2"));
+ final TopicName dest2 = TopicName.get(
+ BrokerTestUtil.newUniqueName("persistent://" + namespace1 +
"/testReplicatorProducerNotExceed2"));
+ admin1.topics().createPartitionedTopic(dest1.toString(), 1);
+ admin1.topicPolicies().setMaxProducers(dest1.toString(), 1);
+ admin2.topics().createPartitionedTopic(dest2.toString(), 1);
+ admin2.topicPolicies().setMaxProducers(dest2.toString(), 1);
+ @Cleanup
+ MessageProducer producer1 = new MessageProducer(url1, dest1);
+ log.info("--- Starting producer1 --- " + url1);
+
+ producer1.produce(1);
+
+ @Cleanup
+ MessageProducer producer2 = new MessageProducer(url2, dest2);
+ log.info("--- Starting producer2 --- " + url2);
+
+ producer2.produce(1);
Review Comment:
It's better to try to add one more producer to make sure new producer is not
allowed to connect to the topic.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/systopic/PartitionedSystemTopicTest.java:
##########
@@ -260,4 +261,33 @@ private void testSetBacklogCausedCreatingProducerFailure()
throws Exception {
Assert.fail("failed to create producer");
}
}
+
+ @Test
+ private void testSystemTopicNotCheckExceed() throws Exception {
Review Comment:
We should also test the max producer limitation for the system topic.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -438,14 +438,21 @@ private PublishRate
publishRateInBroker(ServiceConfiguration config) {
return new PublishRate(config.getMaxPublishRatePerTopicInMessages(),
config.getMaxPublishRatePerTopicInBytes());
}
- protected boolean isProducersExceeded() {
+ protected boolean isProducersExceeded(Producer producer) {
+ if (isSystemTopic() || producer.isRemote()) {
+ return false;
+ }
Integer maxProducers = topicPolicies.getMaxProducersPerTopic().get();
- if (maxProducers > 0 && maxProducers <= producers.size()) {
+ if (maxProducers != null && maxProducers > 0 && maxProducers <=
getUserCreatedProducerSize()) {
return true;
}
return false;
}
+ private long getUserCreatedProducerSize() {
+ return producers.values().stream().filter(p -> !(p.isRemote() ||
p.getTopic().isSystemTopic())).count();
+ }
Review Comment:
Why do we need this change? All the producers under a topic instance should
only point to this topic, right?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
