Skip to content

Conversation

@shibd
Copy link
Member

@shibd shibd commented Mar 12, 2025

Motivation

After #16062, we use a notification machine to discover topic changes for pattern subscriptions.

If the user sets a pattern string like my-property/my-ns/test-pattern.*(without topic domain) when creating a subscription.

It doesn't receive updates for the latest topics because the topic names include the domain when matching.

For example:

  • topicName: persistent://my-property/my-ns/test-pattern-1
  • topicsPattern: my-property/my-ns/test-pattern.*

if (topicsPattern.matcher(TopicName.get(topicName).getPartitionedTopicName()).matches()) {

You can run test to reproduce it: testSubscribePatterWithOutTopicDomain

BTW: This issue has always existed, but some clients had a fallback by periodically querying topics for updates.
For the Java client, this has been removed after #20779, which is why the issue occurs.

Modifications

  • Remove the domain from both the topic name and the topic pattern for matching.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@shibd shibd self-assigned this Mar 12, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 12, 2025
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.23%. Comparing base (bbc6224) to head (2eda62b).
Report is 960 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/apache/pulsar/broker/service/ServerCnx.java 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24072      +/-   ##
============================================
+ Coverage     73.57%   74.23%   +0.66%     
+ Complexity    32624    32428     -196     
============================================
  Files          1877     1863      -14     
  Lines        139502   144291    +4789     
  Branches      15299    16459    +1160     
============================================
+ Hits         102638   107115    +4477     
+ Misses        28908    28719     -189     
- Partials       7956     8457     +501     
Flag Coverage Δ
inttests 26.68% <0.00%> (+2.10%) ⬆️
systests 23.16% <25.00%> (-1.16%) ⬇️
unittests 73.75% <75.00%> (+0.90%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...apache/pulsar/broker/service/TopicListService.java 67.46% <100.00%> (+0.26%) ⬆️
...ava/org/apache/pulsar/common/topics/TopicList.java 100.00% <ø> (+10.34%) ⬆️
...va/org/apache/pulsar/broker/service/ServerCnx.java 72.94% <50.00%> (+0.80%) ⬆️

... and 1060 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also update the javadoc which is now outdated:

* <p>It accepts a regular expression that is compiled into a pattern internally. E.g.,
* "persistent://public/default/pattern-topic-.*"
.
I guess it could be explained that the persistent:// / non-persistent:// prefix is ignored when evaluating the pattern.

@shibd shibd requested a review from lhotari March 13, 2025 13:39
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lhotari
Copy link
Member

lhotari commented Mar 13, 2025

Good work @shibd

@shibd shibd merged commit 3bae1d1 into apache:master Mar 14, 2025
52 checks passed
shibd added a commit that referenced this pull request Mar 14, 2025
…udes the topic domain. (#24072)

(cherry picked from commit 3bae1d1)
shibd added a commit that referenced this pull request Mar 14, 2025
…udes the topic domain. (#24072)

(cherry picked from commit 3bae1d1)
shibd added a commit that referenced this pull request Mar 14, 2025
…udes the topic domain. (#24072)

(cherry picked from commit 3bae1d1)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 21, 2025
…udes the topic domain. (apache#24072)

(cherry picked from commit 3bae1d1)
(cherry picked from commit bc02193)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 21, 2025
…udes the topic domain. (apache#24072)

(cherry picked from commit 3bae1d1)
(cherry picked from commit bc02193)
nodece pushed a commit to nodece/pulsar that referenced this pull request Mar 27, 2025
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 29, 2025
…udes the topic domain. (apache#24072)

(cherry picked from commit 3bae1d1)
(cherry picked from commit 6797678)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 31, 2025
…udes the topic domain. (apache#24072)

(cherry picked from commit 3bae1d1)
(cherry picked from commit 6797678)
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
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.

4 participants