You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/06/07 09:18:39 UTC
[GitHub] [pulsar] yangl opened a new pull request #10850: maxTopicsPerNamespace check should exclude system topic.
yangl opened a new pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850
### Motivation
The current maxTopicsPerNamespace check logic contains system topics, but it should be excluded.
### Modifications
Exclude the 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
[GitHub] [pulsar] yangl commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856655844
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] yangl removed a comment on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl removed a comment on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856458345
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] yangl commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856648620
> @yangl thanks for your contribution. For this PR, do we need to update docs?
No, it doesn't involve docs.
--
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
[GitHub] [pulsar] yangl commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on a change in pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#discussion_r647219725
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
##########
@@ -272,7 +272,7 @@ public static int getPartitionIndex(String topic) {
int partitionIndex = -1;
if (topic.contains(PARTITIONED_TOPIC_SUFFIX)) {
try {
- partitionIndex = Integer.parseInt(topic.substring(topic.lastIndexOf('-') + 1));
+ partitionIndex = Integer.parseInt(StringUtils.substringAfterLast(topic, PARTITIONED_TOPIC_SUFFIX));
Review comment:
ok, rollback first.
--
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
[GitHub] [pulsar] yangl commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856619020
> 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.
--
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
[GitHub] [pulsar] 315157973 commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#discussion_r648819655
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -168,12 +172,19 @@
}
static boolean isSystemTopic(TopicName topicName) {
- if (topicName.isPartitioned()) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME
- .equals(TopicName.get(topicName.getPartitionedTopicName()).getLocalName());
+ if (Objects.equal(SYSTEM_NAMESPACE, topicName.getNamespaceObject())) {
+ return true;
}
+ String localName = TopicName.get(topicName.getPartitionedTopicName()).getLocalName();
+ if (StringUtils.equals(localName, EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME) || StringUtils
+ .equals(localName, EventsTopicNames.TRANSACTION_BUFFER_SNAPSHOT) || (
+ StringUtils.startsWith(localName, EventsTopicNames.TRANSACTION_BUFFER_SNAPSHOT) && StringUtils
+ .endsWith(localName, MLPendingAckStore.PENDING_ACK_STORE_SUFFIX))) {
Review comment:
This does not seem to be a good way. Whenever a new SystemTopic is added, it must be changed here.
--
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
[GitHub] [pulsar] Anonymitaet commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856644915
@yangl thanks for your contribution. For this PR, do we need to update docs?
--
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
[GitHub] [pulsar] yangl commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856458345
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] yangl commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on a change in pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#discussion_r648845290
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -168,12 +172,19 @@
}
static boolean isSystemTopic(TopicName topicName) {
- if (topicName.isPartitioned()) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME
- .equals(TopicName.get(topicName.getPartitionedTopicName()).getLocalName());
+ if (Objects.equal(SYSTEM_NAMESPACE, topicName.getNamespaceObject())) {
+ return true;
}
+ String localName = TopicName.get(topicName.getPartitionedTopicName()).getLocalName();
+ if (StringUtils.equals(localName, EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME) || StringUtils
+ .equals(localName, EventsTopicNames.TRANSACTION_BUFFER_SNAPSHOT) || (
+ StringUtils.startsWith(localName, EventsTopicNames.TRANSACTION_BUFFER_SNAPSHOT) && StringUtils
+ .endsWith(localName, MLPendingAckStore.PENDING_ACK_STORE_SUFFIX))) {
Review comment:
Good idea, add the EVENTS_TOPICS set.
--
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
[GitHub] [pulsar] yangl removed a comment on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl removed a comment on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856655844
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] yangl commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on a change in pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#discussion_r648897991
##########
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:
yeah, moved this to the count logic.
--
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
[GitHub] [pulsar] yangl commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856039569
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] sijie commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-863781348
@315157973 @michaeljmarshall Could you take a look again?
--
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
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#discussion_r654880758
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/events/EventsTopicNames.java
##########
@@ -36,18 +34,18 @@
public static final String NAMESPACE_EVENTS_LOCAL_NAME = "__change_events";
/**
- * Local topic name for the namespace events.
+ * Local topic name for the transaction buffer snapshot.
*/
public static final String TRANSACTION_BUFFER_SNAPSHOT = "__transaction_buffer_snapshot";
/**
* The set of all local topic names declared above.
*/
public static final Set<String> EVENTS_TOPIC_NAMES =
- Collections.unmodifiableSet(
- new HashSet<>(Arrays.asList(NAMESPACE_EVENTS_LOCAL_NAME, TRANSACTION_BUFFER_SNAPSHOT)));
+ Collections.unmodifiableSet(Sets.newHashSet(NAMESPACE_EVENTS_LOCAL_NAME, TRANSACTION_BUFFER_SNAPSHOT));
public static boolean checkTopicIsEventsNames(TopicName topicName) {
- return EVENTS_TOPIC_NAMES.contains(topicName.getLocalName());
+ String name = TopicName.get(topicName.getPartitionedTopicName()).getLocalName();
Review comment:
Since not all system topic names are partitioned, I think it could be worth adding conditional logic to set the `name` variable. The conditional would avoid an unnecessary cache lookup when calling `TopicName.get`. For example, it could look something like this:
```suggestion
String name;
if (topicName.isPartitioned()) {
name = TopicName.get(topicName.getPartitionedTopicName()).getLocalName();
} else {
name = topicName.getLocalName();
}
```
This would be a minor optimization.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -168,12 +170,18 @@
}
static boolean isSystemTopic(TopicName topicName) {
- if (topicName.isPartitioned()) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME
- .equals(TopicName.get(topicName.getPartitionedTopicName()).getLocalName());
+ // event topic
+ if (EventsTopicNames.checkTopicIsEventsNames(topicName)) {
+ return true;
}
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName());
+ String localName = TopicName.get(topicName.getPartitionedTopicName()).getLocalName();
Review comment:
In the case where a topic is _not_ a system topic, we currently calculate the same `localName` twice. Instead of doing it twice (once here and once in the `checkTopicIsEventsNames` method), we could alternatively calculate a `TopicName` without the `-partition-x` prefix first, and then pass it in to the `checkTopicIsEventsNames` method as well as use it below.
This method will be called many times for users that are limiting the number of topics per namespace, so I think we should make sure it is optimized.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -168,12 +170,18 @@
}
static boolean isSystemTopic(TopicName topicName) {
- if (topicName.isPartitioned()) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME
- .equals(TopicName.get(topicName.getPartitionedTopicName()).getLocalName());
+ // event topic
+ if (EventsTopicNames.checkTopicIsEventsNames(topicName)) {
+ return true;
}
- 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)) {
Review comment:
@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.
--
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
[GitHub] [pulsar] BewareMyPower commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856579893
Could you add a test case to cover you 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?
--
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
[GitHub] [pulsar] BewareMyPower commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856522915
> 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.
--
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
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#discussion_r656354387
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -168,12 +170,18 @@
}
static boolean isSystemTopic(TopicName topicName) {
- if (topicName.isPartitioned()) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME
- .equals(TopicName.get(topicName.getPartitionedTopicName()).getLocalName());
+ // event topic
+ if (EventsTopicNames.checkTopicIsEventsNames(topicName)) {
+ return true;
}
- 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)) {
Review comment:
@sijie Could you take a look at @michaeljmarshall 's comment?
BTW, I'll merge this PR first.
--
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
[GitHub] [pulsar] yangl commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856394860
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] BewareMyPower merged pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850
--
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
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by 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
[GitHub] [pulsar] BewareMyPower commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-859363720
@315157973 @michaeljmarshall Could you take a look again?
--
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
[GitHub] [pulsar] yangl commented on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856750355
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] yangl removed a comment on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl removed a comment on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856394860
--
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
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#discussion_r647180668
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java
##########
@@ -272,7 +272,7 @@ public static int getPartitionIndex(String topic) {
int partitionIndex = -1;
if (topic.contains(PARTITIONED_TOPIC_SUFFIX)) {
try {
- partitionIndex = Integer.parseInt(topic.substring(topic.lastIndexOf('-') + 1));
+ partitionIndex = Integer.parseInt(StringUtils.substringAfterLast(topic, PARTITIONED_TOPIC_SUFFIX));
Review comment:
If the behavior is wrong and needs to be fixed, we should open another PR to do it. For example, how could `my-topic-partition-012` be valid? If it's valid, is there any difference between `my-topic-partition-12` and `my-topic-partition-012`? The same goes for `my-topic-partition-0` and `my-topic-partition-00`.
If we need a "correct" implementation, the other language clients, the third party systems that relies on a old version Pulsar, may not be compatible with the latest Pulsar anymore.
--
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
[GitHub] [pulsar] BewareMyPower edited a comment on pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
BewareMyPower edited a comment on pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#issuecomment-856579893
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?
--
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
[GitHub] [pulsar] yangl commented on a change in pull request #10850: maxTopicsPerNamespace check should exclude system topic.
Posted by GitBox <gi...@apache.org>.
yangl commented on a change in pull request #10850:
URL: https://github.com/apache/pulsar/pull/10850#discussion_r654905571
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -168,12 +170,18 @@
}
static boolean isSystemTopic(TopicName topicName) {
- if (topicName.isPartitioned()) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME
- .equals(TopicName.get(topicName.getPartitionedTopicName()).getLocalName());
+ // event topic
+ if (EventsTopicNames.checkTopicIsEventsNames(topicName)) {
+ return true;
}
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName());
+ String localName = TopicName.get(topicName.getPartitionedTopicName()).getLocalName();
Review comment:
Thank you for your advice. Done.
--
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