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/04/17 23:09:54 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #10254: [Broker] Remove possible error case for system topic name checking
michaeljmarshall opened a new pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254
### Motivation
In reading through the system topic code, I noticed that there are some edge cases where normal topics might be considered system topics. This PR should remove that possibility.
I have a few questions on implementation. I will put comments on the lines where I have questions.
### Modifications
1. Updated the `isSystemTopic` method to just check for presence the system topic prefix (`__`). This change could be problematic if end users are already using topic names with the system topic prefix.
2. Use a topic's `localName` to check for equality instead of `endsWith`. In the current implementation, a user could have a topic with the local name `my__change_events`, and because it "ends with" `__change_events`, it would be considered a system topic.
3. Update the documentation on topic names to warn about the system topic prefix.
### Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
### Documentation
System topics were introduced in pulsar 2.6.0. I updated the documentation for all versions since then to indicate that users should avoid creating topics that begin with `__`. This is important because if there are to be additional system topics, we want to make sure there are not accidentally naming collisions.
--
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] congbobo184 commented on a change in pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#discussion_r615871300
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -167,8 +167,11 @@
SystemTopicClient<T> getSystemTopic();
}
+ /**
+ * A topic is a system topic if and only if its local name has the defined prefix.
+ */
static boolean isSystemTopic(TopicName topicName) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName());
+ return topicName.getLocalName().startsWith(EventsTopicNames.SYSTEM_TOPIC_LOCAL_NAME_PREFIX);
Review comment:
i don't think `__` would be considered system topics is a good idea. It will cause problem when broker upgrade .
--
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 #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#discussion_r615315203
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -167,8 +167,11 @@
SystemTopicClient<T> getSystemTopic();
}
+ /**
+ * A topic is a system topic if and only if its local name has the defined prefix.
+ */
static boolean isSystemTopic(TopicName topicName) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName());
+ return topicName.getLocalName().startsWith(EventsTopicNames.SYSTEM_TOPIC_LOCAL_NAME_PREFIX);
Review comment:
The one major consequence of this implementation is the all topics created or existing with `__` would be considered system topics. Perhaps that is too much of a breaking change.
I updated it this way because the old implementation ignored the `TRANSACTION_BUFFER_SNAPSHOT` topic, which is a system topic. It seems easier to maintain if we just follow the paradigm instead of adding every new system topic name as we create new ones.
--
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] congbobo184 commented on a change in pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#discussion_r615854206
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -643,7 +643,8 @@ protected void handleProducerRemoved(Producer producer) {
}
try {
- if (!topic.endsWith(EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME)
+ // TODO can all system topics have any type of subscription?
+ if (!TopicName.get(topic).getLocalName().equals(EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME)
Review comment:
`TRANSACTION_BUFFER_SNAPSHOT` does not need to include this check
--
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] congbobo184 commented on a change in pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#discussion_r615871300
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -167,8 +167,11 @@
SystemTopicClient<T> getSystemTopic();
}
+ /**
+ * A topic is a system topic if and only if its local name has the defined prefix.
+ */
static boolean isSystemTopic(TopicName topicName) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName());
+ return topicName.getLocalName().startsWith(EventsTopicNames.SYSTEM_TOPIC_LOCAL_NAME_PREFIX);
Review comment:
i don't think `__` would be considered system topics is a good idea. It will cause version incompatibility.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -643,7 +643,8 @@ protected void handleProducerRemoved(Producer producer) {
}
try {
- if (!topic.endsWith(EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME)
+ // TODO can all system topics have any type of subscription?
+ if (!TopicName.get(topic).getLocalName().equals(EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME)
Review comment:
TRANSACTION_BUFFER_SNAPSHOT does not need to include this check
--
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 #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#issuecomment-831661817
@michaeljmarshall have you gotten a change to address the comments?
--
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] codelipenghui commented on pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#issuecomment-846326365
Hi @michaeljmarshall, Could you please help confirm if https://github.com/apache/pulsar/pull/10529 has fixed the problem? Sorry, sorry we missed your PR.
--
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 #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#discussion_r615315074
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -643,7 +643,8 @@ protected void handleProducerRemoved(Producer producer) {
}
try {
- if (!topic.endsWith(EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME)
+ // TODO can all system topics have any type of subscription?
+ if (!TopicName.get(topic).getLocalName().equals(EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME)
Review comment:
`TRANSACTION_BUFFER_SNAPSHOT` was not included in this check. Should it be?
--
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 closed pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall closed pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254
--
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 #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#discussion_r615315057
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -2523,7 +2523,7 @@ public boolean isAllowAutoTopicCreation(final String topic) {
public boolean isAllowAutoTopicCreation(final TopicName topicName) {
//System topic can always be created automatically
- if (EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName())
+ if (SystemTopicClient.isSystemTopic(topicName)
Review comment:
`TRANSACTION_BUFFER_SNAPSHOT` was previously not included in this check. I think it is likely preferred to allow all system topics to be auto created.
--
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 edited a comment on pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
sijie edited a comment on pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#issuecomment-831661817
@michaeljmarshall have you gotten a chance to address the comments?
--
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 pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#issuecomment-822866380
> Why did the tests now actually run for this PR? There are code changes for this PR as well.
Great question. I took a look and didn't see anything obviously wrong. Will look again later tonight (my time).
@lhotari - this PR skipped tests for some reason. The PR includes both documentation and code changes. Tagging you, as I know you're familiar with the logic used to skip tests. Thanks.
--
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 pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#issuecomment-831700209
@sijie and @congbobo184 - I just added a commit addressing the comments. However, I am wondering if this is the right change. While rebasing, I noticed that #10334 shows that there are possibly more system topics than I realized. When initially writing the PR, I had assumed that the `EventsTopicNames` class contained all system topics, however, that might not be true. Let me know if this PR needs more changes.
--
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] jerrypeng commented on pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#issuecomment-822708402
Why did the tests now actually run for this PR? There are code changes for this PR as well.
--
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 #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#discussion_r622726142
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/systopic/SystemTopicClient.java
##########
@@ -167,8 +167,11 @@
SystemTopicClient<T> getSystemTopic();
}
+ /**
+ * A topic is a system topic if and only if its local name has the defined prefix.
+ */
static boolean isSystemTopic(TopicName topicName) {
- return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName());
+ return topicName.getLocalName().startsWith(EventsTopicNames.SYSTEM_TOPIC_LOCAL_NAME_PREFIX);
Review comment:
That is a great point. It does seem like there is already a bit of a trend where system topics begin with `__`, but you're right that it would need to be introduced more intentionally than this PR.
I'll look at changing this PR accordingly.
--
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 pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#issuecomment-863475997
I'd like to see better definition around system topics. I don't think this PR will achieve that though, so I am going to close this for now.
--
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 pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#issuecomment-857877922
> Hi @michaeljmarshall, Could you please help confirm if #10529 has fixed the problem? Sorry, sorry we missed your PR.
@codelipenghui - based on a quick look through that PR, I don't think it fixes the problem. My main concern is that checking that a topic's suffix matches a string is not the same as checking that a topic's name matches a string. I would like to see stricter rules for pulsar system topics. Sorry, I haven't had time to come back to this PR to address some of the comments. Hopefully I'll be able to get back to this PR soon.
--
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] lhotari commented on a change in pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#discussion_r648510777
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/events/EventsTopicNames.java
##########
@@ -18,29 +18,34 @@
*/
package org.apache.pulsar.common.events;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
/**
- * System topic name for the event type.
+ * System topic names for each {@link EventType}.
*/
public class EventsTopicNames {
+ /**
+ * All event topics are system topics, and currently, they all have this prefix.
+ */
+ private static final String SYSTEM_TOPIC_LOCAL_NAME_PREFIX = "__";
/**
* Local topic name for the namespace events.
*/
- public static final String NAMESPACE_EVENTS_LOCAL_NAME = "__change_events";
+ public static final String NAMESPACE_EVENTS_LOCAL_NAME = SYSTEM_TOPIC_LOCAL_NAME_PREFIX + "change_events";
/**
* Local topic name for the namespace events.
*/
- public static final String TRANSACTION_BUFFER_SNAPSHOT = "__transaction_buffer_snapshot";
+ public static final String TRANSACTION_BUFFER_SNAPSHOT =
+ SYSTEM_TOPIC_LOCAL_NAME_PREFIX + "transaction_buffer_snapshot";
- public static boolean checkTopicIsEventsNames(String topicName) {
- if (topicName.endsWith(NAMESPACE_EVENTS_LOCAL_NAME)) {
- return true;
- } else if (topicName.endsWith(TRANSACTION_BUFFER_SNAPSHOT)) {
- return true;
- } else {
- return false;
- }
- }
+ /**
+ * The set of all events topic names.
+ */
+ public static final Set<String> EVENTS_TOPIC_NAMES =
+ new HashSet<>(Arrays.asList(NAMESPACE_EVENTS_LOCAL_NAME, TRANSACTION_BUFFER_SNAPSHOT));
Review comment:
this should be wrapped with `Collections.unmodifiableSet` to make the `Set` immutable.
--
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 pull request #10254: [Broker] Remove possible error case for system topic name checking
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #10254:
URL: https://github.com/apache/pulsar/pull/10254#issuecomment-821900649
@codelipenghui and @congbobo184 - would you mind taking a look at this PR? I believe both of you have worked on the different system topics features. Thanks!
--
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