-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 namespace policy override ignored when creating subscription #12699
Conversation
.defaultNumPartitions(3) | ||
.build()); | ||
|
||
admin.topics().createSubscription(topicString + "-partition-0", subscriptionName, MessageId.earliest); |
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.
Do you want to subscribe a to specific partition?
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 that's the intent. I've seen it done here so I guess it's OK.
Also this way the topic is created by pulsar().getBrokerService().getTopic()
in internalCreateSubscriptionForNonPartitionedTopic
instead of pulsar().getBrokerService().fetchPartitionedTopicMetadataCheckAllowAutoCreationAsync()
when you don't specify a partition.
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.
Sorry, I don't understand why change the namespace level policy for topic auto-creation to create partitioned topic with 3 partitions, but here check if able to subscribe to one partition, looks like not able to ensure the partitioned topic has been created?
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.
Oh, I see. This test doesn't really make sense. I removed it.
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.
Note that currently it's possible to create a topic partition from the subscription creation without the full partitioned topic being created. Is it OK to be able to do that ?
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.
@codelipenghui is it OK for you ?
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
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
eebbe33
to
6c5e81f
Compare
PR apache#6471 introduced the possibility to set auto topic creation settings at the namespace level PR apache#6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in apache#6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation
6c5e81f
to
cda14f4
Compare
) PR #6471 introduced the possibility to set auto topic creation settings at the namespace level PR #6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in #6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation (cherry picked from commit 965a80f)
) PR #6471 introduced the possibility to set auto topic creation settings at the namespace level PR #6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in #6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation (cherry picked from commit 965a80f)
…che#12699) PR apache#6471 introduced the possibility to set auto topic creation settings at the namespace level PR apache#6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in apache#6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation (cherry picked from commit 965a80f) (cherry picked from commit a034d14)
…che#12699) PR apache#6471 introduced the possibility to set auto topic creation settings at the namespace level PR apache#6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in apache#6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation
Motivation
PR #6471 introduced the possibility to set auto topic creation settings at the namespace level
PR #6685 is a fix to take into account the auto topic creation setting when creating a subscription
but it uses the broker configuration and doesn't see the namespace level setting that was introduced in #6471
Modifications
This fix uses the
BrokerService
isAllowAutoTopicCreation
method that fetches config overrides in ZK instead of the one from the broker configuration.This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation
Verifying this change
This change added tests and can be verified as follows:
PersistentTopicsBase.java
and the tests won't pass anymoreDoes this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
doc-required
no-need-doc
It's a fix
doc