[fix][broker] Resolve replicator remote cluster by prefix so cluster names containing a dot work - #26451
Open
SEPURI-SAI-KRISHNA wants to merge 1 commit into
Conversation
…names containing a dot work Cluster names may contain dots (NamedEntity#NAMED_ENTITY_PATTERN allows "-=:." plus \w), but AbstractReplicator#getRemoteCluster recovered the cluster from a replicator cursor name by splitting on "." and taking the last segment, while getReplicatorName builds that name as <replicatorPrefix>.<remoteCluster>. The two were not inverses: for a cluster "remote.east" the cursor "pulsar.repl.remote.east" resolved to "east". Every admin operation addressed at such a replicator subscription failed with a 404, and PersistentTopic#removeOrphanReplicationCursors mistook the live replicator for an orphan on every topic load. Strip the known replicator prefix instead of splitting on ".", making getRemoteCluster the exact inverse of getReplicatorName. All call sites already guard with startsWith(replicatorPrefix), so the prefix is in scope at each of them. Assisted-by: Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #26450
Motivation
Cluster names are allowed to contain
.—NamedEntity.NAMED_ENTITY_PATTERNis^[-=:.\w]*$,and the comment above it states this applies to "property, namespace, cluster and topic names".
AbstractReplicatorbuilds replicator cursor/subscription names as<replicatorPrefix>.<remoteCluster>ingetReplicatorName, but recovered the cluster by splittingon
.and taking the last segment:The two are therefore not inverses. For a cluster named
remote.east, the cursorpulsar.repl.remote.eastresolves toeast. Two consequences:Every admin operation addressed at a replicator subscription fails. Six call sites parse the
name, then look the replicator up under the wrong cluster and get
null— clear-backlog, skipmessages, expire messages by position, expire messages by timestamp,
getReplicatorReference(peek and replicator stats), and namespace-wide clear-backlog. The first five return
404 Replicator not foundfor a subscription thattopics statsplainly lists; the sixth targetsthe wrong subscription. Each already guards with
subName.startsWith(replicatorPrefix)on theline immediately above, so the subscription is recognised as a replicator's — only the cluster
it belongs to is derived wrongly.
The orphan-cursor sweep misidentifies live replicators.
PersistentTopic#removeOrphanReplicationCursors()runs insideinitialize(), i.e. on every topicload.
getRemoteCluster("pulsar.repl.remote.east")returnedeast, which is not among the topic'sreplication clusters, so a live, correctly-configured replicator was declared orphaned and
removeReplicator("east")was called. That rebuilds the name aspulsar.repl.eastand callsasyncDeleteCursoron it; no such cursor exists, so the callback fails withCursorNotFoundExceptionand theinitialize()chain fails. The cursor survived only because thereconstructed name was wrong too — the sweep fully intended to delete a cursor that was not
orphaned. #22890 documents that wrongly removing a replicator cursor loses the entire replication
backlog, so this sits on a code path already known to be destructive when it misfires.
Modifications
AbstractReplicator#getRemoteClusternow takes the replicator prefix and strips it, making itthe exact inverse of
getReplicatorName(replicatorPrefix, cluster). A name that does not carrythe prefix is returned unchanged, so callers fail their replicator lookup exactly as before.
PersistentTopic,PersistentTopicsBaseandNamespacesBase.All of them already had the prefix in scope from the
startsWithguard, so no plumbing wasneeded beyond passing it in.
Note on the signature: this replaces the single-argument
public static getRemoteCluster(String)rather than adding an overload. Keeping the old one as a deprecated delegate is not possible
without the prefix — the missing prefix is the bug — and leaving a knowingly-wrong method in
place seemed worse than removing it. Happy to reconsider if a deprecated overload is preferred for
broker-plugin compatibility.
Fixing the parsing was chosen over rejecting dotted cluster names: deployments may already use
them, and turning those into a validation error would break working clusters on upgrade.
Verifying this change
This change added tests and can be verified as follows:
AbstractReplicatorTest#testGetRemoteClusterRoundTripsClusterNamesContainingDots— assertsgetRemoteCluster(getReplicatorName(prefix, cluster)) == clusterforus-west,us-east.prod,a.b.c,cluster:1andr3.AbstractReplicatorTest#testGetRemoteClusterLeavesNonReplicatorNamesUnchanged— a name withoutthe prefix is returned untouched.
PersistentTopicTest#testReplicatorCursorOfClusterWithDotInNameIsNotTreatedAsOrphan— creates atopic with a live replicator cursor for cluster
remote.east, loads it, and asserts the orphansweep logs no removal warning and the cursor survives. This test fails on unpatched code with
the live replicator of cluster remote.east was treated as an orphan: [Remove the orphan replicator because the cluster does not exist].Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
The REST endpoints and admin CLI options are unchanged in shape; the affected operations simply
stop returning 404 for replicator subscriptions of dotted cluster names.
Documentation
doc-requireddoc-not-neededdocdoc-completeBug fix; no documented behaviour changes.
Matching PR in forked repository
PR in forked repository: N/A (branch built and tested locally; broker tests listed above pass)
Assisted-by: Claude Code