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
[fix][broker] Fix NPE when set AutoTopicCreationOverride
#15653
Conversation
throw new RestException(Status.NOT_ACCEPTABLE, | ||
"Number of partitions should be less than or equal to " + maxPartitions); | ||
if (Objects.equals(autoTopicCreationOverride.getTopicType(), TopicType.PARTITIONED.toString())) { | ||
if (maxPartitions > 0 && autoTopicCreationOverride.getDefaultNumPartitions() > maxPartitions) { |
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.
if the problem is that autoTopicCreationOverride.getDefaultNumPartitions() can be null then we have to simply add a null check and fail the request if it is not passed while maxPartitions is > 0
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.
A simple null check cannot clearly express why it is null here, which will increase the burden on the maintainers later.
I think a better approach is to use logical judgments to avoid null checks everywhere.
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.
But you should add the null check anyway. Because here we can fail with NPE
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 null check already exists in bellow code:
Lines 39 to 62 in 3c60063
public static ValidateResult validateOverride(AutoTopicCreationOverride override) { | |
if (override == null) { | |
return ValidateResult.fail("[AutoTopicCreationOverride] can not be null"); | |
} | |
if (override.isAllowAutoTopicCreation()) { | |
if (!TopicType.isValidTopicType(override.getTopicType())) { | |
return ValidateResult.fail(String.format("Unknown topic type [%s]", override.getTopicType())); | |
} | |
if (TopicType.PARTITIONED.toString().equals(override.getTopicType())) { | |
if (override.getDefaultNumPartitions() == null) { | |
return ValidateResult.fail("[defaultNumPartitions] cannot be null when the type is partitioned."); | |
} | |
if (override.getDefaultNumPartitions() <= 0) { | |
return ValidateResult.fail("[defaultNumPartitions] cannot be less than 1 for partition type."); | |
} | |
} else if (TopicType.NON_PARTITIONED.toString().equals(override.getTopicType())) { | |
if (override.getDefaultNumPartitions() != null) { | |
return ValidateResult.fail("[defaultNumPartitions] is not allowed to be" | |
+ " set when the type is non-partition."); | |
} | |
} | |
} | |
return ValidateResult.success(); | |
} |
/pulsarbot run-failure-checks |
(cherry picked from commit e2afcf0)
(cherry picked from commit e2afcf0)
Remove the |
Restore the |
(cherry picked from commit e2afcf0)
Does the cherry-picked tags means it has been cherry-picked in the 2.10 version ? |
@Nowadays Yes. |
Motivation
When the user wants to set the
AutoTopicCreationOverride
policy withnon-partitioned
type, the correct request body is as below.On the broker side, the request body entity field
defaultNumPartitions
isnull
. the NPE will occur when the user sets configmaxNumPartitionsPerPartitionedTopic
greater than 0.pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
Lines 839 to 842 in 3c60063
Modifications
maxNumPartitionsPerPartitionedTopic
justtopicType
ispartitioned
Verifying this change
Add a test
testAutoTopicCreationOverrideWithMaxNumPartitionsLimit
to verify this change.Documentation
no-need-doc