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

[improve] [client] improve the class GetTopicsResult #22766

Merged
merged 7 commits into from
May 30, 2024

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented May 23, 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

I will submit three PRs to solve the two issues above.

  • (The current PR) Improve the GetTopicsResult, so we can get the original partitions of partitioned topics
  • (Following PR) Multi Topics Consumer only subscribes to the existing partitions if the config createTopicIfDoesNotExist is false.
  • (Following PR) The Patten consumer that is started only removes the partitions that were deleted.

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/2.11.5 release/3.2.3 release/3.0.6 labels May 23, 2024
@poorbarcode poorbarcode added this to the 3.4.0 milestone May 23, 2024
@poorbarcode poorbarcode self-assigned this May 23, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 23, 2024
@poorbarcode poorbarcode requested a review from gaoran10 May 23, 2024 08:00
@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 82.50000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 73.22%. Comparing base (bbc6224) to head (f0ce2b2).
Report is 301 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22766      +/-   ##
============================================
- Coverage     73.57%   73.22%   -0.35%     
+ Complexity    32624    32611      -13     
============================================
  Files          1877     1890      +13     
  Lines        139502   141522    +2020     
  Branches      15299    15534     +235     
============================================
+ Hits         102638   103633     +995     
- Misses        28908    29885     +977     
- Partials       7956     8004      +48     
Flag Coverage Δ
inttests 27.45% <0.00%> (+2.86%) ⬆️
systests 24.78% <45.00%> (+0.46%) ⬆️
unittests 72.23% <82.50%> (-0.62%) ⬇️

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

Files Coverage Δ
...g/apache/pulsar/client/impl/HttpLookupService.java 84.11% <100.00%> (+2.86%) ⬆️
...ava/org/apache/pulsar/common/naming/TopicName.java 96.09% <100.00%> (+0.03%) ⬆️
...e/pulsar/client/impl/BinaryProtoLookupService.java 83.82% <0.00%> (+1.28%) ⬆️
...g/apache/pulsar/common/lookup/GetTopicsResult.java 88.46% <92.00%> (+25.96%) ⬆️
...a/org/apache/pulsar/client/impl/LookupService.java 80.00% <66.66%> (ø)

... and 368 files with indirect coverage changes

@poorbarcode poorbarcode changed the title [improve] [client] add an internal API lookupService.getExistsPartitions [improve] [client] improve the class GetTopicsResult May 29, 2024
@poorbarcode poorbarcode merged commit 87a3339 into apache:master May 30, 2024
51 checks passed
lhotari pushed a commit that referenced this pull request Jun 4, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2024
(cherry picked from commit 87a3339)
(cherry picked from commit 250dacb)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2024
(cherry picked from commit 87a3339)
(cherry picked from commit 250dacb)
poorbarcode added a commit that referenced this pull request Jun 11, 2024
poorbarcode added a commit that referenced this pull request Jun 11, 2024
poorbarcode added a commit that referenced this pull request Jul 11, 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.2 cherry-picked/branch-3.3 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.11.5 release/3.0.6 release/3.2.4 release/3.3.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.

6 participants