maxTopicsPerNamespace check should exclude system topic. by yangl · Pull Request #10850 · apache/pulsar

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.

yangl

done.

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

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.

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

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

-        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

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

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

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

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 315157973, aahmed-se, addisonj, eolivelli, hangc0276 and merlimat June 10, 2021 03:07

315157973 reviewed Jun 10, 2021

View reviewed changes

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

minor refactor.

67aa21b

michaeljmarshall reviewed Jun 10, 2021

View reviewed changes

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

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

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

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

Uh oh!

Conversation

yangl commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Uh oh!

BewareMyPower commented Jun 8, 2021

Uh oh!

Uh oh!

yangl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BewareMyPower commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yangl commented Jun 8, 2021

Uh oh!

BewareMyPower left a comment

Choose a reason for hiding this comment

Uh oh!

Anonymitaet commented Jun 8, 2021

Uh oh!

yangl commented Jun 8, 2021

Uh oh!

yangl commented Jun 8, 2021

Uh oh!

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BewareMyPower commented Jun 11, 2021

Uh oh!

sijie commented Jun 18, 2021

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaeljmarshall Jun 20, 2021

Choose a reason for hiding this comment

Uh oh!

BewareMyPower Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yangl commented Jun 7, 2021 •

edited

Loading

BewareMyPower commented Jun 8, 2021 •

edited

Loading