Skip to content

Conversation

@Jason918
Copy link
Contributor

@Jason918 Jason918 commented Dec 3, 2021

Motivation

This is one of the serial topic policy optimization with HierarchyTopicPolicies.

Update topic policy with HierarchyTopicPolicies comes with these benefits:

  • All topic policy related settings will goes into AbstractTopic#topicPolicies, easier to understand, check, review, and modify.
  • Unify policy update to AbstractTopic. And easier to find which policy is not applied to non-persistent topic yet. And we can easily add support for it.
  • Unify policy value with 3 level settings (topic/namespace/broker), and priority with topic > namespace > broker. And it's easier to find which level settings is missing.
  • All values are updated at creation or by trigger. We can save some resource to update it or recalculate each time we use it.
  • etc.

Modifications

Add new field subscriptionTypesEnabled in org.apache.pulsar.broker.service.AbstractTopic#topicPolicies.

ServiceConfiguration#subscriptionTypesEnabled is dynamic, this is solved by BrokerService#registerConfigurationListener.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as

  • org.apache.pulsar.broker.admin.TopicPoliciesTest#testSubscriptionTypesEnabled
  • org.apache.pulsar.broker.admin.NamespacesTest#testSubscriptionTypesEnabled

Some checklist for updating topic policy with HierarchyTopicPolicies.

  • Broker level value set. And if it's dynamic (yes).
  • Namespace level value updated.
  • Topic level value updated.
  • Pre-existing unit test covers this change and updated to verify this change.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc

Code optimization.

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 3, 2021
@Jason918 Jason918 force-pushed the opt_tp_subscriptionTypesEnabled branch from 131d6fc to 772744d Compare December 3, 2021 14:20
@Jason918
Copy link
Contributor Author

Jason918 commented Dec 4, 2021

/pulsarbot run-failure-checks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants