-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[improve][broker] Add dedicated partition configuration for system topics to reduce resource usage #23785
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
base: master
Are you sure you want to change the base?
[improve][broker] Add dedicated partition configuration for system topics to reduce resource usage #23785
Changes from all commits
594ec35
d04fe96
586d1fc
5b52438
a1d41fa
3984650
9155035
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3729,9 +3729,11 @@ public boolean isDefaultTopicTypePartitioned(final TopicName topicName, final Op | |
| public int getDefaultNumPartitions(final TopicName topicName, final Optional<Policies> policies) { | ||
| AutoTopicCreationOverride autoTopicCreationOverride = getAutoTopicCreationOverride(topicName, policies); | ||
| if (autoTopicCreationOverride != null) { | ||
| return autoTopicCreationOverride.getDefaultNumPartitions(); | ||
| return isSystemTopic(topicName) ? autoTopicCreationOverride.getSystemTopicDefaultNumPartitions() | ||
| : autoTopicCreationOverride.getDefaultNumPartitions(); | ||
| } else { | ||
| return pulsar.getConfiguration().getDefaultNumPartitions(); | ||
| return isSystemTopic(topicName) ? pulsar.getConfiguration().getSystemTopicDefaultNumPartitions() | ||
| : pulsar.getConfiguration().getDefaultNumPartitions(); | ||
|
Comment on lines
+3735
to
+3736
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should check that |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ public final class AutoTopicCreationOverrideImpl implements AutoTopicCreationOve | |
| private boolean allowAutoTopicCreation; | ||
| private String topicType; | ||
| private Integer defaultNumPartitions; | ||
| private Integer systemTopicDefaultNumPartitions; | ||
|
|
||
| public static ValidateResult validateOverride(AutoTopicCreationOverride override) { | ||
| if (override == null) { | ||
|
|
@@ -51,6 +52,14 @@ public static ValidateResult validateOverride(AutoTopicCreationOverride override | |
| if (override.getDefaultNumPartitions() <= 0) { | ||
| return ValidateResult.fail("[defaultNumPartitions] cannot be less than 1 for partition type."); | ||
| } | ||
| if (override.getSystemTopicDefaultNumPartitions() == null) { | ||
| return ValidateResult.fail( | ||
| "[systemTopicDefaultNumPartitions] cannot be null when the type is partitioned."); | ||
| } | ||
| if (override.getSystemTopicDefaultNumPartitions() <= 0) { | ||
| return ValidateResult.fail( | ||
| "[systemTopicDefaultNumPartitions] cannot be less than 1 for partition type."); | ||
| } | ||
|
Comment on lines
+55
to
+62
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wouldn't be backwards compatible for existing deployments of Pulsar. Please handle backwards compatibility in some way.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One possible way would be to use defaultNumPartitions in the case that |
||
| } else if (TopicType.NON_PARTITIONED.toString().equals(override.getTopicType())) { | ||
| if (override.getDefaultNumPartitions() != null) { | ||
| return ValidateResult.fail("[defaultNumPartitions] is not allowed to be" | ||
|
|
@@ -69,6 +78,7 @@ public static class AutoTopicCreationOverrideImplBuilder implements AutoTopicCre | |
| private boolean allowAutoTopicCreation; | ||
| private String topicType; | ||
| private Integer defaultNumPartitions; | ||
| private Integer systemTopicDefaultNumPartitions; | ||
|
|
||
| public AutoTopicCreationOverrideImplBuilder allowAutoTopicCreation(boolean allowAutoTopicCreation) { | ||
| this.allowAutoTopicCreation = allowAutoTopicCreation; | ||
|
|
@@ -85,8 +95,15 @@ public AutoTopicCreationOverrideImplBuilder defaultNumPartitions(Integer default | |
| return this; | ||
| } | ||
|
|
||
| public AutoTopicCreationOverrideImplBuilder systemTopicDefaultNumPartitions( | ||
| Integer systemTopicDefaultNumPartitions) { | ||
| this.systemTopicDefaultNumPartitions = systemTopicDefaultNumPartitions; | ||
| return this; | ||
| } | ||
|
|
||
| public AutoTopicCreationOverrideImpl build() { | ||
| return new AutoTopicCreationOverrideImpl(allowAutoTopicCreation, topicType, defaultNumPartitions); | ||
| return new AutoTopicCreationOverrideImpl(allowAutoTopicCreation, topicType, defaultNumPartitions, | ||
| systemTopicDefaultNumPartitions); | ||
| } | ||
| } | ||
| } | ||
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.
It would be useful to support existing way where the default number of partitions is determined by defaultNumPartitions, for example setting this value to
-1.For backwards compatibility the default value should be
-1when this change is introduced. We could make the default1in master branch for the feature release of Pulsar.