[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.

[GitBox <gi...@apache.org>]

michaeljmarshall commented on a change in pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#discussion_r648873908



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -168,12 +172,23 @@
     }
 
     static boolean isSystemTopic(TopicName topicName) {
-        if (topicName.isPartitioned()) {
-            return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME
-                    .equals(TopicName.get(topicName.getPartitionedTopicName()).getLocalName());
+        // system namespace

Review comment:
       I agree that the logic in this method is insufficient. I initially proposed a similar change here: https://github.com/apache/pulsar/pull/10254. In general, I think it would be valuable to take a closer look at how we manage system topics. It'd be great to have a standard way to define system topics so that we don't have to enumerate them in these types of checks, especially because I expect that we'll add more system topics over time.

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -638,7 +639,7 @@ protected void internalCreatePartitionedTopic(AsyncResponse asyncResponse, int n
                 maxTopicsPerNamespace = pulsar().getConfig().getMaxTopicsPerNamespace();
             }
 
-            if (maxTopicsPerNamespace > 0) {
+            if (maxTopicsPerNamespace > 0 && !SystemTopicClient.isSystemTopic(topicName)) {

Review comment:
       Same as the `BrokerService` class comment.

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -2608,7 +2608,7 @@ private boolean isSystemTopic(String topic) {
                 maxTopicsPerNamespace = pulsar.getConfig().getMaxTopicsPerNamespace();
             }
 
-            if (maxTopicsPerNamespace > 0) {
+            if (maxTopicsPerNamespace > 0 && !SystemTopicClient.isSystemTopic(topicName)) {

Review comment:
       I don't think the logic here is sufficient to prevent the counting of system topics in the overall topic count for a namespace. On line 2614, we get _all_ topics, which will include system topics in the count of current topics. I think that we'll need to also filter out topics that qualify as system topics.




-- 
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