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
[Transaction] Fix transaction system topic create in loop. #12749
[Transaction] Fix transaction system topic create in loop. #12749
Conversation
String topic = topicName.toString(); | ||
return topic.startsWith(TopicName.TRANSACTION_COORDINATOR_ASSIGN.toString()) | ||
|| topic.startsWith(TopicName.get(TopicDomain.persistent.value(), | ||
NamespaceName.SYSTEM_NAMESPACE, TRANSACTION_LOG_PREFIX).toString()) | ||
|| topic.endsWith(MLPendingAckStore.PENDING_ACK_STORE_SUFFIX); | ||
} | ||
|
||
public static boolean isNotAllowedToCreateTopic(TopicName topicName) { | ||
String topic = topicName.toString(); | ||
return topic.startsWith(TopicName.get(TopicDomain.persistent.value(), |
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.
what about creating a constant for this ? "TopicName.get(TopicDomain.persistent.value(), NamespaceName.SYSTEM_NAMESPACE, TRANSACTION_LOG_PREFIX).toString()"
@@ -244,6 +245,13 @@ protected void validateAdminAndClientPermission() { | |||
} | |||
} | |||
|
|||
protected void validateTopicAllowdToCreate(TopicName 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.
typo: validateCreateTopic
Can you add my reproducer as a test case ? I am not sure that we will really fix the problem. Because we are not forbidding to create a topic with a system 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.
I left one last minor comment.
|| topic.endsWith(MLPendingAckStore.PENDING_ACK_STORE_SUFFIX); | ||
} | ||
|
||
public static boolean isNotAllowedToCreateTopic(TopicName 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.
what about naming this method isTransactionInternalName
?
the meaning is that this is a reserved name
I see that you are using this method while listing the topic names, so "Create" is not good in that place
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.
LGTM
…ate_transaction_system_topic
Confirmed w/ @codelipenghui, this PR is a bug fix, no need to update docs. |
) fix apache#12727 ### Motivation Now transaction system topic can be created. ### Modifications we should not allow broker or user create by transaction system format topic. 1. checkout topic auto create. 2. admin create topic. ### Verifying this change add some test for it
fix #12727 ### Motivation Now transaction system topic can be created. ### Modifications we should not allow broker or user create by transaction system format topic. 1. checkout topic auto create. 2. admin create topic. ### Verifying this change add some test for it (cherry picked from commit 2c4d913)
fix #12727
Motivation
Now transaction system topic can be created.
Modifications
we should not allow broker or user create by transaction system format topic.
Verifying this change
add some test for it