-
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] Change create topic return error to Status.BAD_REQUEST #12919
[Broker] Change create topic return error to Status.BAD_REQUEST #12919
Conversation
@congbobo184:Thanks for your contribution. For this PR, do we need to update docs? |
@congbobo184:Thanks for providing doc info! |
@@ -250,7 +250,7 @@ protected void validateAdminAndClientPermission() { | |||
protected void validateCreateTopic(TopicName topicName) { | |||
if (isTransactionInternalName(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.
The topic TRANSACTION_COORDINATOR_ASSIGN
could be created by users, right?
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.
yes
@@ -250,7 +250,7 @@ protected void validateAdminAndClientPermission() { | |||
protected void validateCreateTopic(TopicName topicName) { | |||
if (isTransactionInternalName(topicName)) { | |||
log.warn("Try to create a topic in the system topic format! {}", topicName); | |||
throw new RestException(Status.CONFLICT, "Cannot create topic in system topic format!"); | |||
throw new RestException(Status.BAD_REQUEST, "Cannot create topic in system topic format!"); |
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.
Could we list the system topics here? Users could know which are system topics.
if (e.getCause() instanceof NotAllowedException) { | ||
throw new RestException(Status.CONFLICT, e.getCause()); | ||
throw new RestException(Status.BAD_REQUEST, e.getCause()); |
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.
Interesting, do we through NotAllowedException
for pulsar().getBrokerService().getTopicIfExists()
? @congbobo184 Could you please help check how to improve this part? From the method name, we only get the topic ref if the topic exists, but looks like it also create topics.
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.
ok
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.
+1
@@ -250,7 +250,7 @@ protected void validateAdminAndClientPermission() { | |||
protected void validateCreateTopic(TopicName topicName) { | |||
if (isTransactionInternalName(topicName)) { | |||
log.warn("Try to create a topic in the system topic format! {}", 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.
change the log.warn information to log.warn("Forbidden to create transaction internal topic: {}" topicName)
maybe more clear.
if (e.getCause() instanceof NotAllowedException) { | ||
throw new RestException(Status.CONFLICT, e.getCause()); | ||
throw new RestException(Status.BAD_REQUEST, e.getCause()); |
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.
+1
@congbobo184 Could you please check the comments? |
…c_error_code_change
…c_error_code_change
…c_error_code_change
…c_error_code_change
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
(cherry picked from commit 2262a5d)
Motivation
When create transaction system topic, broker will return Status.CONFLICT, It does not conform to the wrong return semantics of the method. So change it to Status.BAD_REQUEST, when client receive Status.BAD_REQUEST, it will generic PulsarAdminException and error code is Status.BAD_REQUEST
Modifications
change the error code to Status.BAD_REQUEST when create transaction system topic.
Verifying this change
change test verify