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][client] Fix pattern consumer create crash if a part of partitions of a topic have been deleted #22854

Merged
merged 30 commits into from
Jul 9, 2024

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Jun 5, 2024

Motivation

Background

1.Regarding the API getting topics by regexp pattern, there are two implementations:*

  • List<String> HttpLookupService.getPartitionedTopicMetadata(...)
  • CommandGetTopicsOfNamespace BinaryProtoLookupService.getPartitionedTopicMetadata(...)

Pulsar transferred both response types List<String> and CommandGetTopicsOfNamespace to a GetTopicsResult object. And discarded the partition information when doing transference. For example:

  • Get a list topic-1-partition-0, topic-1-partition-1.
  • The transferring operation will group them to topic-1.

2.The behavior of Patten consumers


Issue

  1. When users are starting a Pattern consumer, the consumer will try to subscribe to all the partitions even if some partitions have been deleted before, and then the client crashes due to a Topic Not Exists Exception. You can reproduce the issue by the test testConsumerAfterOnePartDeleted
  2. The Patten consumer that is started removes all partitions when one partition was deleted, even if there are still half of the partitions exist.

Modifications

  • Multi Topics Consumer only subscribes to the existing partitions if the config createTopicIfDoesNotExist is false.
  • The Patten consumer that is started only removes the partitions that were deleted.
  • Fix race condition issues of updating subscriptions

Addational things

flink-connector-pulsar also has the same issue

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode self-assigned this Jun 5, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 5, 2024
@poorbarcode poorbarcode added type/bug The PR fixed a bug or issue reported a bug release/2.11.5 release/3.3.1 release/3.0.6 and removed doc-not-needed Your PR changes do not impact docs labels Jun 5, 2024
@poorbarcode poorbarcode added this to the 3.4.0 milestone Jun 5, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 5, 2024
@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@poorbarcode
Copy link
Contributor Author

Rebased master

@poorbarcode poorbarcode requested a review from lhotari June 19, 2024 13:35
@lhotari lhotari changed the title [fix] [client] Fix pattern consumer create crash if a part of partitions of a topic have been deleted [fix][client] Fix pattern consumer create crash if a part of partitions of a topic have been deleted Jun 25, 2024
@lhotari
Copy link
Member

lhotari commented Jun 25, 2024

Closing and reopening to trigger CI with latest master branch changes.

@lhotari lhotari closed this Jun 25, 2024
@lhotari lhotari reopened this Jun 25, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 73.77049% with 80 lines in your changes missing coverage. Please review.

Project coverage is 73.44%. Comparing base (bbc6224) to head (8a7bafa).
Report is 443 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22854      +/-   ##
============================================
- Coverage     73.57%   73.44%   -0.13%     
- Complexity    32624    33337     +713     
============================================
  Files          1877     1913      +36     
  Lines        139502   143383    +3881     
  Branches      15299    15637     +338     
============================================
+ Hits         102638   105308    +2670     
- Misses        28908    30038    +1130     
- Partials       7956     8037      +81     
Flag Coverage Δ
inttests 27.77% <6.88%> (+3.19%) ⬆️
systests 24.69% <15.73%> (+0.36%) ⬆️
unittests 72.50% <73.77%> (-0.35%) ⬇️

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

Files Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerBase.java 74.21% <100.00%> (+0.09%) ⬆️
...a/org/apache/pulsar/client/impl/LookupService.java 0.00% <ø> (ø)
...rg/apache/pulsar/client/impl/TopicListWatcher.java 66.91% <100.00%> (-0.95%) ⬇️
...g/apache/pulsar/common/lookup/GetTopicsResult.java 92.50% <100.00%> (+30.00%) ⬆️
...ava/org/apache/pulsar/common/naming/TopicName.java 96.09% <100.00%> (+0.03%) ⬆️
...java/org/apache/pulsar/common/util/FutureUtil.java 76.61% <50.00%> (+2.06%) ⬆️
...rg/apache/pulsar/client/impl/PulsarClientImpl.java 73.94% <12.50%> (-0.37%) ⬇️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 77.58% <71.66%> (-0.15%) ⬇️
...pulsar/client/impl/PatternConsumerUpdateQueue.java 79.61% <79.61%> (ø)
...ar/client/impl/PatternMultiTopicsConsumerImpl.java 76.88% <69.64%> (-6.58%) ⬇️

... and 487 files with indirect coverage changes

@codelipenghui codelipenghui merged commit 9626e7e into apache:master Jul 9, 2024
51 checks passed
poorbarcode added a commit that referenced this pull request Jul 11, 2024
…ns of a topic have been deleted (#22854)

(cherry picked from commit 9626e7e)
poorbarcode added a commit that referenced this pull request Jul 11, 2024
…ns of a topic have been deleted (#22854)

(cherry picked from commit 9626e7e)
poorbarcode added a commit that referenced this pull request Jul 11, 2024
…ns of a topic have been deleted (#22854)

(cherry picked from commit 9626e7e)
poorbarcode added a commit that referenced this pull request Jul 11, 2024
…ns of a topic have been deleted (#22854)

(cherry picked from commit 9626e7e)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 12, 2024
…ns of a topic have been deleted (apache#22854)

(cherry picked from commit 9626e7e)
(cherry picked from commit 98d4a53)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 15, 2024
…ns of a topic have been deleted (apache#22854)

(cherry picked from commit 9626e7e)
(cherry picked from commit 98d4a53)
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.

7 participants