lhotari commented on code in PR #23785:
URL: https://github.com/apache/pulsar/pull/23785#discussion_r2773497017
##########
pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/impl/AutoTopicCreationOverrideImpl.java:
##########
@@ -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.");
+ }
Review Comment:
This wouldn't be backwards compatible for existing deployments of Pulsar.
Please handle backwards compatibility in some way.
##########
pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/impl/AutoTopicCreationOverrideImpl.java:
##########
@@ -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.");
+ }
Review Comment:
One possible way would be to use defaultNumPartitions in the case that
`systemTopicDefaultNumPartitions` is null or `<= 0`
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -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();
Review Comment:
this should check that `systemTopicDefaultNumPartitions > 0`.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2343,6 +2343,12 @@ The max allowed delay for delayed delivery (in
milliseconds). If the broker rece
+ " if allowAutoTopicCreationType is partitioned."
)
private int defaultNumPartitions = 1;
+ @FieldContext(
+ category = CATEGORY_STORAGE_ML,
+ dynamic = true,
+ doc = "Default number of partitions for the system topics."
+ )
+ private int systemTopicDefaultNumPartitions = 1;
Review Comment:
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 `-1` when this
change is introduced. We could make the default `1` in master branch for the
feature release of Pulsar.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
