-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Broker] Remove possible error case for system topic name checking #10254
[Broker] Remove possible error case for system topic name checking #10254
Conversation
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TRANSACTION_BUFFER_SNAPSHOT
was not included in this check. Should it be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TRANSACTION_BUFFER_SNAPSHOT does not need to include this check
static boolean isSystemTopic(TopicName topicName) { | ||
return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName()); | ||
return topicName.getLocalName().startsWith(EventsTopicNames.SYSTEM_TOPIC_LOCAL_NAME_PREFIX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think __
would be considered system topics is a good idea. It will cause problem when broker upgrade .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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! |
9302eeb
to
11f26ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good work! left some comment.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TRANSACTION_BUFFER_SNAPSHOT does not need to include this check
static boolean isSystemTopic(TopicName topicName) { | ||
return EventsTopicNames.NAMESPACE_EVENTS_LOCAL_NAME.equals(topicName.getLocalName()); | ||
return topicName.getLocalName().startsWith(EventsTopicNames.SYSTEM_TOPIC_LOCAL_NAME_PREFIX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think __
would be considered system topics is a good idea. It will cause problem when broker upgrade .
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. |
@michaeljmarshall have you gotten a chance to address the comments? |
11f26ae
to
89a0043
Compare
89a0043
to
1e68628
Compare
@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 |
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. |
* 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be wrapped with Collections.unmodifiableSet
to make the Set
immutable.
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. |
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
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.localName
to check for equality instead ofendsWith
. In the current implementation, a user could have a topic with the local namemy__change_events
, and because it "ends with"__change_events
, it would be considered a system topic.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.