Skip to content
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] [broker] Fix can not subscribe partitioned topic with a suffix-matched regexp #22025

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

poorbarcode
Copy link
Contributor

Motivation

If a consumer tries to subscribe to a partitioned topic with a suffix-matched regexp, it does not work.

  • create topic persistent://public/default/tp
  • create a consumer with regexp persistent://public/default/tp$
  • the pattern consumer has no internal consumers.

You can reproduce the issue by the test testPreciseRegexpSubscribe.

  • it can work when using 2.10.x
  • it can not work when using >=2.11.0, it is a break change due to PIP-145

#21885 fixed one wrong logic here, but there is still another wrong logic, see https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/TopicListService.java#L64

@Override
public void accept(String topicName, NotificationType notificationType) {
    // The param "topicName" contains the partition suffix, which causes a suffix-matched regexp not to match.
    if (topicsPattern.matcher(topicName).matches()) {
        List<String> newTopics;
        List<String> deletedTopics;
    ...

Why can the test testPreciseRegexpSubscribe pass?

#21885 added a test testPreciseRegexpSubscribe that covers the test which creates a consumer with a suffix-matched regexp. It works as below:

The test testPreciseRegexpSubscribe can not pass as the below flow:

  • create a consumer with a suffix-matched regexp
  • wait for Topic List Watcher to start.
    • the consumer will call recheckTopicsChangeAfterReconnect at this step.
  • create a partitioned topic
    • the consumer can not receive the event due to the wrong logic, then the test failed.

Modifications

  • Review all the codes of the Topic List Watcher and add descriptions of the topic name.
  • Fix the wrong behavior.

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added type/bug The PR fixed a bug or issue reported a bug category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost release/3.0.3 release/2.11.4 release/3.1.3 release/3.2.1 labels Feb 5, 2024
@poorbarcode poorbarcode added this to the 3.3.0 milestone Feb 5, 2024
@poorbarcode poorbarcode self-assigned this Feb 5, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 5, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fce0717) 72.90% compared to head (09410ea) 73.58%.
Report is 22 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22025      +/-   ##
============================================
+ Coverage     72.90%   73.58%   +0.68%     
- Complexity    32464    32562      +98     
============================================
  Files          1850     1874      +24     
  Lines        138611   139221     +610     
  Branches      15201    15260      +59     
============================================
+ Hits         101052   102448    +1396     
+ Misses        29711    28849     -862     
- Partials       7848     7924      +76     
Flag Coverage Δ
inttests 24.62% <0.00%> (?)
systests 24.36% <0.00%> (?)
unittests 72.85% <100.00%> (-0.05%) ⬇️

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

Files Coverage Δ
...apache/pulsar/broker/resources/TopicResources.java 96.61% <ø> (ø)
...ache/pulsar/broker/namespace/NamespaceService.java 72.23% <ø> (+1.02%) ⬆️
...pulsar/broker/service/PulsarCommandSenderImpl.java 87.62% <ø> (ø)
...apache/pulsar/broker/service/TopicListService.java 67.20% <100.00%> (ø)
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 78.08% <ø> (ø)
...ar/client/impl/PatternMultiTopicsConsumerImpl.java 84.21% <ø> (+0.75%) ⬆️
...rg/apache/pulsar/client/impl/TopicListWatcher.java 68.57% <ø> (+1.42%) ⬆️
...ar/client/impl/conf/ConsumerConfigurationData.java 92.55% <ø> (ø)
...va/org/apache/pulsar/common/protocol/Commands.java 90.59% <ø> (ø)

... and 209 files with indirect coverage changes

@poorbarcode poorbarcode merged commit b375e86 into apache:master Feb 19, 2024
71 of 74 checks passed
Technoboy- pushed a commit that referenced this pull request Feb 20, 2024
Technoboy- pushed a commit that referenced this pull request Feb 22, 2024
Technoboy- pushed a commit that referenced this pull request Feb 27, 2024
poorbarcode added a commit that referenced this pull request Feb 28, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost cherry-picked/branch-2.11 cherry-picked/branch-3.0 cherry-picked/branch-3.1 cherry-picked/branch-3.2 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.11.4 release/3.0.3 release/3.1.3 release/3.2.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants