maxTopicsPerNamespace check should exclude system topic. #10850

yangl · 2021-06-07T09:18:33Z

Motivation

The current maxTopicsPerNamespace check logic contains system topics, but it should be excluded. As the even if we turn off allowAutoTopicCreation, the systemtopic will auto creation too.

Modifications

Exclude the system topics for the maxTopicsPerNamespace logic.
SystemTopic add the missed logic, such as __transaction_buffer_snapshot 、start with the __transaction_buffer_snapshot end with the __transaction_pending_ack.

In addition, why is mytopic-partition--1 is a partitioned topic（with --）? Which version of us uses this logic?

        // NOTE: Following behavior is not right actually, but for the backward compatibility, it shouldn't be changed
        assertEquals(TopicName.getPartitionIndex("mytopic-partition--1"), 1);
        assertEquals(TopicName.getPartitionIndex("mytopic-partition-00"), 0);
        assertEquals(TopicName.getPartitionIndex("mytopic-partition-012"), 12);

this testcase added with dd8be83#diff-445b0cfa56ca0c784df78e73d9294f2a37f079ca3c15c345b03c09d56f81ebffR204 by @BewareMyPower

Signed-off-by: YANGLiiN <ielin@qq.com>

BewareMyPower · 2021-06-08T07:20:00Z

In addition, why is mytopic-partition--1 is a partitioned topic（with --）? Which version of us uses this logic?

This comment was marked by me. Though it's a wrong behavior, like -partition-00 and -partition-012 should not be treated as a valid partition, if it was changed, the other client may not be compatible with the broker anymore.

pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java

yangl

done.

pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java

BewareMyPower · 2021-06-08T08:41:57Z

Could you add a test case to cover your change? Your test only covers the isSystemTopic method.

I mean changing AdminApiTest2#testMaxTopicsPerNamespace. Could you manually create some system topics and see if the test could be affected?

yangl · 2021-06-08T09:35:25Z

Could you add a test case to cover your change? Your test only covers the isSystemTopic method.

I mean changing AdminApiTest2#testMaxTopicsPerNamespace. Could you manually create some system topics and see if the test could be affected?

done.

BewareMyPower

LGTM

Anonymitaet · 2021-06-08T10:17:12Z

@yangl thanks for your contribution. For this PR, do we need to update docs?

yangl · 2021-06-08T10:25:23Z

@yangl thanks for your contribution. For this PR, do we need to update docs?

No, it doesn't involve docs.

yangl · 2021-06-08T13:06:24Z

/pulsarbot run-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java

michaeljmarshall

I agree that system topics shouldn't count against the max topic limit per namespace. I left some comments for potential edge cases that I think need to be considered.

pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java

# Conflicts: # pulsar-common/src/main/java/org/apache/pulsar/common/events/EventsTopicNames.java

BewareMyPower · 2021-06-11T07:54:34Z

@315157973 @michaeljmarshall Could you take a look again?

sijie · 2021-06-18T06:06:20Z

@315157973 @michaeljmarshall Could you take a look again?

michaeljmarshall

@yangl - thanks for addressing my initial comments. Those changes look good. I left some comments on optimizations I think we should implement.

pulsar-common/src/main/java/org/apache/pulsar/common/events/EventsTopicNames.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java

michaeljmarshall · 2021-06-20T06:47:02Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java

-        return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName());
+        String localName = TopicName.get(topicName.getPartitionedTopicName()).getLocalName();
+        // transaction pending ack topic
+        if (StringUtils.endsWith(localName, MLPendingAckStore.PENDING_ACK_STORE_SUFFIX)) {


@sijie - I am concerned that this logic technically includes user topics. That technically leaves users open to accidentally (or intentionally) creating topics that could be classified as system topics. Given the change in this PR, letting a topic end in this special suffix could allow users to create topics and exceed a namespace's topic limit (assuming the limit is enabled). I don't think this comment should prevent us from merging this PR, but I do think we should address this concern at some point.

@sijie Could you take a look at @michaeljmarshall 's comment?

BTW, I'll merge this PR first.

Signed-off-by: YANGLiiN <ielin@qq.com>

michaeljmarshall

@yangl - thanks for addressing my comments. LGTM.

eolivelli

LGTM

@BewareMyPower

### Motivation The current maxTopicsPerNamespace check logic contains system topics, but it should be excluded. As the even if we turn off `allowAutoTopicCreation`, the systemtopic will auto creation too. ### Modifications 1. Exclude the system topics for the maxTopicsPerNamespace logic. 2. SystemTopic add the missed logic, such as `__transaction_buffer_snapshot` 、start with the `__transaction_buffer_snapshot` end with the `__transaction_pending_ack`. In addition, why is mytopic-partition--1 is a partitioned topic（with --）? Which version of us uses this logic? ```java // NOTE: Following behavior is not right actually, but for the backward compatibility, it shouldn't be changed assertEquals(TopicName.getPartitionIndex("mytopic-partition--1"), 1); assertEquals(TopicName.getPartitionIndex("mytopic-partition-00"), 0); assertEquals(TopicName.getPartitionIndex("mytopic-partition-012"), 12); ``` this testcase added with apache@dd8be83#diff-445b0cfa56ca0c784df78e73d9294f2a37f079ca3c15c345b03c09d56f81ebffR204 by @BewareMyPower

@BewareMyPower

### Motivation The current maxTopicsPerNamespace check logic contains system topics, but it should be excluded. As the even if we turn off `allowAutoTopicCreation`, the systemtopic will auto creation too. ### Modifications 1. Exclude the system topics for the maxTopicsPerNamespace logic. 2. SystemTopic add the missed logic, such as `__transaction_buffer_snapshot` 、start with the `__transaction_buffer_snapshot` end with the `__transaction_pending_ack`. In addition, why is mytopic-partition--1 is a partitioned topic（with --）? Which version of us uses this logic? ```java // NOTE: Following behavior is not right actually, but for the backward compatibility, it shouldn't be changed assertEquals(TopicName.getPartitionIndex("mytopic-partition--1"), 1); assertEquals(TopicName.getPartitionIndex("mytopic-partition-00"), 0); assertEquals(TopicName.getPartitionIndex("mytopic-partition-012"), 12); ``` this testcase added with apache@dd8be83#diff-445b0cfa56ca0c784df78e73d9294f2a37f079ca3c15c345b03c09d56f81ebffR204 by @BewareMyPower

maxTopicsPerNamespace check should exclude system topic.

d041c13

yangl marked this pull request as draft June 7, 2021 10:00

add testcase for isSystemTopic.

8980776

yangl marked this pull request as ready for review June 7, 2021 11:32

yangl marked this pull request as draft June 7, 2021 13:39

yangl added 3 commits June 7, 2021 21:44

fix the LICENSE check.

18ca5be

Signed-off-by: YANGLiiN <ielin@qq.com>

fix the mytopic-partition--1 testcase.

d5627c5

add pulsar/system the namespace.

5e3d7cd

BewareMyPower requested changes Jun 8, 2021

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java Outdated Show resolved Hide resolved

yangl marked this pull request as ready for review June 8, 2021 07:28

rollback TopicName partitionIndex

c2c1059

yangl commented Jun 8, 2021

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java Outdated Show resolved Hide resolved

add testcase for system topic setMaxTopicsPerNamespace

e0bb175

BewareMyPower approved these changes Jun 8, 2021

View reviewed changes

BewareMyPower assigned yangl Jun 8, 2021

BewareMyPower added the area/admin label Jun 8, 2021

yangl mentioned this pull request Jun 10, 2021

[Broker] Always let system topic TRANSACTION_BUFFER_SNAPSHOT be auto created #10876

Merged

BewareMyPower requested review from hangc0276, merlimat, eolivelli, 315157973, aahmed-se and addisonj June 10, 2021 03:07

315157973 reviewed Jun 10, 2021

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java Outdated Show resolved Hide resolved

minor refactor.

67aa21b

michaeljmarshall reviewed Jun 10, 2021

View reviewed changes

yangl added 5 commits June 10, 2021 14:49

exclude system topic

4bc7c60

Merge branch 'apache_master' into maxTopicsPerNamespace

c8fc0a9

# Conflicts: # pulsar-common/src/main/java/org/apache/pulsar/common/events/EventsTopicNames.java

merge EventsTopicNames

f4dd887

use checkEventTopic

59d2b5f

fix checkstyle.

e4e1e0d

yangl marked this pull request as draft June 10, 2021 07:46

max check logic for created different to new create .

fd24b6c

yangl marked this pull request as ready for review June 10, 2021 10:35

yangl requested a review from 315157973 June 10, 2021 10:37

sijie approved these changes Jun 18, 2021

View reviewed changes

sijie added this to the 2.9.0 milestone Jun 18, 2021

michaeljmarshall reviewed Jun 20, 2021

View reviewed changes

optimize twice calculate localName.

91d5789

Signed-off-by: YANGLiiN <ielin@qq.com>

michaeljmarshall approved these changes Jun 21, 2021

View reviewed changes

eolivelli approved these changes Jun 22, 2021

View reviewed changes

BewareMyPower merged commit dbcaa98 into apache:master Jun 22, 2021

yangl deleted the maxTopicsPerNamespace branch June 23, 2021 01:10

wuzhanpeng mentioned this pull request Aug 3, 2021

add test for auto-created partitioned system topic #11545

Merged

michaeljmarshall mentioned this pull request May 24, 2023

[fix][broker] partitioned __change_events topic is policy topic #20392

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maxTopicsPerNamespace check should exclude system topic. #10850

maxTopicsPerNamespace check should exclude system topic. #10850

yangl commented Jun 7, 2021 •

edited

Loading

BewareMyPower commented Jun 8, 2021

yangl left a comment

BewareMyPower commented Jun 8, 2021 •

edited

Loading

yangl commented Jun 8, 2021

BewareMyPower left a comment

Anonymitaet commented Jun 8, 2021

yangl commented Jun 8, 2021

yangl commented Jun 8, 2021

michaeljmarshall left a comment

BewareMyPower commented Jun 11, 2021

sijie commented Jun 18, 2021

michaeljmarshall left a comment

michaeljmarshall Jun 20, 2021

BewareMyPower Jun 22, 2021

michaeljmarshall left a comment

eolivelli left a comment

maxTopicsPerNamespace check should exclude system topic. #10850

maxTopicsPerNamespace check should exclude system topic. #10850

Conversation

yangl commented Jun 7, 2021 • edited Loading

Motivation

Modifications

BewareMyPower commented Jun 8, 2021

yangl left a comment

Choose a reason for hiding this comment

BewareMyPower commented Jun 8, 2021 • edited Loading

yangl commented Jun 8, 2021

BewareMyPower left a comment

Choose a reason for hiding this comment

Anonymitaet commented Jun 8, 2021

yangl commented Jun 8, 2021

yangl commented Jun 8, 2021

michaeljmarshall left a comment

Choose a reason for hiding this comment

BewareMyPower commented Jun 11, 2021

sijie commented Jun 18, 2021

michaeljmarshall left a comment

Choose a reason for hiding this comment

michaeljmarshall Jun 20, 2021

Choose a reason for hiding this comment

BewareMyPower Jun 22, 2021

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

yangl commented Jun 7, 2021 •

edited

Loading

BewareMyPower commented Jun 8, 2021 •

edited

Loading