Skip to content

[improve][broker] Load topic policies on non-persistent topic load and gate the policy replay#26134

Merged
lhotari merged 11 commits into
apache:masterfrom
lhotari:lh-disable-topic-listener-replay
Jul 2, 2026
Merged

[improve][broker] Load topic policies on non-persistent topic load and gate the policy replay#26134
lhotari merged 11 commits into
apache:masterfrom
lhotari:lh-disable-topic-listener-replay

Conversation

@lhotari

@lhotari lhotari commented Jul 2, 2026

Copy link
Copy Markdown
Member

Motivation

Topic policies are applied to a topic when it loads via AbstractTopic#initTopicPolicy (added for
persistent topics in #16144), but non-persistent topics never did this: NonPersistentTopic.initialize()
completed its policy-listener wrapper with null policies and relied on a namespace-wide broadcast to
receive its policies. That broadcast — the end-of-topic "replay" in
SystemTopicBasedTopicPoliciesService.initPolicesCache — also had two bugs: it iterated the entire
policiesCache (every namespace, not just the one being loaded), and it ignored globalPoliciesCache
entirely. So on any namespace load it re-notified every topic's listener across all namespaces and never
replayed global policies.

Modifications

  • Move initTopicPolicy() and the topicPolicyListener wrapper up from PersistentTopic to
    AbstractTopic, and call it from NonPersistentTopic.initialize() (guarded by an exceptionally like
    PersistentTopic, so a policy-cache load failure/timeout doesn't fail topic creation). Non-persistent
    topics now load and apply their own global/local policies on load. Removed the now-unused
    AbstractTopic.registerTopicPolicyListener().
  • Because each topic now loads its own policies on load, the namespace-wide replay is redundant. Extract
    replayTopicPolicyListeners(namespace) that replays only the loaded namespace's topics and includes both
    local and global cached policies, and gate it behind a new ServiceConfiguration flag
    topicPolicyListenerReplayEnabled (disabled by default). The replay is only needed for custom plugins
    that register TopicPolicyListeners and depend on the broadcast; such deployments can enable the flag for
    backwards compatibility.
  • Since policies are now applied immediately on load, TopicPolicyListenerWrapper.completeInitialization
    emits the local policy before the global one, so a local topic policy takes precedence at apply time (for
    example, it avoids creating a compaction subscription for a global compaction policy when the local policy
    disables compaction). When no local policy exists, nothing is emitted for the local scope, so global-only
    topics are unaffected.

Verifying this change

This change added tests and can be verified as follows:

  • TopicPoliciesTest.testNonPersistentTopicAppliesTopicPolicyOnLoad: sets a topic policy, unloads, then
    loads a non-persistent topic fresh and asserts the policy is applied on load (fails on the previous
    behavior).
  • SystemTopicBasedTopicPoliciesServiceTest.testReplayTopicPolicyListenersNotifiesOnlyNamespaceScopedLocalAndGlobalPolicies
    and testTopicPolicyListenerReplayDisabledByDefault: assert the replay is namespace-scoped, includes both
    local and global policies, and is off by default.
  • TopicPolicyListenerWrapperTest: updated to assert the local-before-global emit order, plus a case that
    no local scope is emitted when there is no local policy.

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Adds a new broker configuration topicPolicyListenerReplayEnabled (default false).

lhotari added 4 commits July 2, 2026 06:12
…-persistent topics

PR apache#16144 added initTopicPolicy for PersistentTopic but not for non-persistent topics: a
NonPersistentTopic registered its policy listener in the constructor and completed the listener wrapper
with null policies, so it never applied its own topic-level policies on load and instead depended on the
namespace-wide policy broadcast.

Move the topicPolicyListener wrapper field and initTopicPolicy() up to AbstractTopic
(getTopicPolicyListener() now returns the wrapper) and have NonPersistentTopic.initialize() call
initTopicPolicy(), so a non-persistent topic registers its listener and applies its initial global and
local policies on load, exactly like a persistent topic. This removes the need to re-broadcast every
topic's policy when a namespace's policy cache finishes loading (handled in a follow-up commit).

Assisted-by: Claude Code (Opus 4.8)
…ayEnabled

When a namespace's __change_events reader reached the end during the initial cache load,
SystemTopicBasedTopicPoliciesService re-notified topic-policy listeners for every cached policy. This had
three problems: it iterated the whole policiesCache (all namespaces, not just the one being loaded), it
ignored globalPoliciesCache entirely, and it is redundant now that every topic loads its own policies on
load (AbstractTopic#initTopicPolicy).

Extract replayTopicPolicyListeners(namespace) that replays only the loaded namespace's topics and includes
both local and global policies, and gate it behind a new ServiceConfiguration flag
topicPolicyListenerReplayEnabled (disabled by default). The replay is only needed for custom plugins that
register TopicPolicyListeners and rely on the broadcast; such deployments can enable the flag for backwards
compatibility.

Assisted-by: Claude Code (Opus 4.8)
AbstractTopic#initTopicPolicy now loads and applies a topic's policies on load (for non-persistent topics
too), and updates are applied immediately. Because a policy update takes effect at apply time, the order in
which the initial global and local policies are emitted matters: applying the global policy first can let a
global-only setting take effect that a present local policy should override -- for example, a compaction
subscription being created for a global compaction policy even though the local policy disables compaction.

Emit the local policy before the global policy in TopicPolicyListenerWrapper.completeInitialization, so that
by the time the global policy is applied the local override is already in place and the merged (local-wins)
result is what takes effect. This does not fully solve every apply-order hazard, but it removes them whenever
a local policy exists. When no local policy exists, nothing is emitted for the local scope, so this is safe
and does not change behavior for topics that only have a global policy.

Assisted-by: Claude Code (Opus 4.8)
@nodece

nodece commented Jul 2, 2026

Copy link
Copy Markdown
Member

Question about topicPolicyListenerReplayEnabled configuration

The core improvements in this PR are solid. However, I have a question about whether the topicPolicyListenerReplayEnabled configuration is actually necessary.

Reasoning:

  1. All topics (both persistent and non-persistent) now call initTopicPolicy() during initialization, which already notifies registered listeners with the loaded policies.

  2. The TopicPoliciesService interface contract requires listeners to be idempotent and handle duplicate notifications.

  3. The replay mechanism simply re-notifies listeners with the same policy values they already received during topic load.

Question: Is there a specific use case where the replay provides value beyond the per-topic notifications that already occur during topic initialization? If not, consider removing this configuration to simplify the codebase.

The rest of the changes look well-designed and properly tested.

@lhotari

lhotari commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Question: Is there a specific use case where the replay provides value beyond the per-topic notifications that already occur during topic initialization? If not, consider removing this configuration to simplify the codebase.

The replay is only needed for custom third party Pulsar plugins that register TopicPolicyListeners and rely on the broadcast; such deployments can enable the flag for backwards compatibility. Since it's unlikely that anyone uses it, the configuration flag has only be added to ServiceConfiguration and not to the broker.conf/standalone.conf files.
org.apache.pulsar.broker.service.TopicPoliciesService has @InterfaceAudience.LimitedPrivate annotation, so it's not a supported interface for plugins. We can handle the removal for Pulsar 5.0, but since this PR targets also 4.0.x and 4.2.x, it's better to not introduce breaking changes.

@lhotari lhotari marked this pull request as draft July 2, 2026 09:16
lhotari added 3 commits July 2, 2026 12:41
getTopicPoliciesAsync short-circuits via isSelf() for the __change_events system topic, so a __change_events
topic loading its own policies (AbstractTopic#initTopicPolicy, now shared by persistent and non-persistent
topics) returns empty without creating a policy-cache reader on __change_events -- which would otherwise be a
recursive, deadlocking dependency. Add an explicit regression test for that guard.

Assisted-by: Claude Code (Opus 4.8)
@lhotari lhotari marked this pull request as ready for review July 2, 2026 12:01
@lhotari lhotari marked this pull request as draft July 2, 2026 12:58
…ner wrapper

Restructure AbstractTopic#initTopicPolicy and TopicPolicyListenerWrapper so the wrapper's initialization is
always completed and the method can be run again cleanly.

- TopicPolicyListenerWrapper#startInitialization() begins a fresh buffering phase (resetting any previous
  one), and completeInitialization() is now idempotent (a no-op once completed). This makes initTopicPolicy
  re-runnable, which is groundwork for adding a retry later -- the retry itself is not part of this change.
  Initialization runs are serialized, so concurrent phases are not supported.

- initTopicPolicy runs startInitialization() up front and, in a terminal whenComplete handler, always calls
  completeInitializationUnlessAlreadyCompleted(). Previously the wrapper was completed on the success path and,
  on error, only when an exception occurred; other cases (for example the listener not being registered) could
  leave it buffering forever and dropping updates. Now the wrapper always leaves the buffering phase and
  forwards the buffered value plus all future live updates, while the (possibly failed) future is still
  returned so the caller's initialize() logs and continues.

- The "initialization took too long" warning now measures from the start of the current phase.

Assisted-by: Claude Code (Opus 4.8)
@lhotari lhotari marked this pull request as ready for review July 2, 2026 13:42
lhotari added 2 commits July 2, 2026 16:51
…ocal precedence

Unlike the other topic-policy fields, which are stored in PolicyHierarchyValue (local and global values kept
separately, local winning), AbstractTopic.subscriptionPolicies was a plain map assigned directly in
updateTopicPolicy(). With the local-before-global initialization order (see TopicPolicyListenerWrapper), the
local subscription policies were applied first and then overwritten by the global map; since TopicPolicies
defaults subscriptionPolicies to an empty map, a global policy without subscription-level overrides would clear
the local per-subscription policies during initialization (e.g. a per-subscription dispatch-rate policy).

Store the local and global per-subscription policies separately and recompute the effective map by merging them
with local precedence: a subscription present in the local policies keeps its local value, otherwise the global
value (if any) is used. This preserves the local policies through initialization and through later global
updates, and falls back to the global value when a local entry is absent.

Assisted-by: Claude Code (Opus 4.8)
@lhotari lhotari merged commit 0629dc5 into apache:master Jul 2, 2026
44 of 45 checks passed
@lhotari lhotari added this to the 5.0.0-M2 milestone Jul 2, 2026
lhotari added a commit that referenced this pull request Jul 2, 2026
…d gate the policy replay (#26134)

(cherry picked from commit 0629dc5)
lhotari added a commit that referenced this pull request Jul 3, 2026
…d gate the policy replay (#26134)

(cherry picked from commit 0629dc5)
lhotari added a commit that referenced this pull request Jul 3, 2026
…n idle non-persistent topics

### Motivation

On branch-4.0, topic-level policy admin operations run `PersistentTopicsBase#preValidation`,
which checks topic existence via `AdminResource#checkTopicExistsAsync`. For non-persistent
topics that check lists topics through `NamespaceService#getListOfNonPersistentTopics`, which
only returns "active" non-persistent topics (those with producers, consumers or subscriptions).
A freshly created or otherwise idle non-persistent topic has none of these, so it was reported
as non-existent and the operation failed with `404 NOT_FOUND` -- for example
`admin.topicPolicies().setMaxSubscriptionsPerTopic(topic, 10)` immediately after
`admin.topics().createNonPartitionedTopic(topic)`.

This is branch-4.0 specific. On master and branch-4.2 it is already fixed by #24225
("Allow recreation of partitioned topic after metadata loss"), which rewrote
`checkTopicExistsAsync` to delegate to `NamespaceService#checkTopicExistsAsync`. #24225 was
never backported to branch-4.0.

Surfaced by `TopicPoliciesTest#testNonPersistentTopicAppliesTopicPolicyOnLoad` (added by #26134),
which fails on branch-4.0 but passes on master/branch-4.2.

### Modifications

In `AdminResource#checkTopicExistsAsync`, resolve non-persistent topic existence through
`NamespaceService#checkTopicExistsAsync`, which detects a loaded non-persistent topic via the
owner broker's in-memory topic map (no "active" filter). Persistent-topic handling is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants