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] Optimize code for MultiTopicsConsumerImpl #18748

Merged
merged 1 commit into from
Dec 8, 2022

Conversation

Pomelongan
Copy link
Contributor

@Pomelongan Pomelongan commented Dec 5, 2022

Motivation

In org.apache.pulsar.client.impl.MultiTopicsConsumerImpl#createPartitionedConsumer, it is more appropriate to change some places to generic format. For example, see line 932, it is better to changeConsumerConfigurationData to ConsumerConfigurationData<T>.

ConsumerConfigurationData cloneConf = conf.clone();
String topicName = cloneConf.getSingleTopic();
cloneConf.getTopicNames().remove(topicName);
CompletableFuture<Consumer> future = new CompletableFuture<>();
MultiTopicsConsumerImpl consumer = new MultiTopicsConsumerImpl(client, topicName, cloneConf, executorProvider,
future, schema, interceptors, true /* createTopicIfDoesNotExist */);
future.thenCompose(c -> ((MultiTopicsConsumerImpl) c).subscribeAsync(topicName, numPartitions))

Modifications

Change some code in org.apache.pulsar.client.impl.MultiTopicsConsumerImpl#createPartitionedConsumer to generic mode.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: Pomelongan#16

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

@Pomelongan Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@Pomelongan Pomelongan changed the title optimize code [improve][client] Optimize code for MultiTopicsConsumerImpl Dec 5, 2022
@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Dec 5, 2022
@AnonHxy AnonHxy added this to the 2.12.0 milestone Dec 6, 2022
@codelipenghui
Copy link
Contributor

@Pomelongan Please merge/rebase the master branch. There is a flaky test that has been fixed. #18751

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2022

Codecov Report

Merging #18748 (31c8b87) into master (68ca60c) will decrease coverage by 2.26%.
The diff coverage is 11.11%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18748      +/-   ##
============================================
- Coverage     50.05%   47.78%   -2.27%     
+ Complexity    11024    10657     -367     
============================================
  Files           703      703              
  Lines         68814    68816       +2     
  Branches       7378     7377       -1     
============================================
- Hits          34446    32886    -1560     
- Misses        30621    32208    +1587     
+ Partials       3747     3722      -25     
Flag Coverage Δ
unittests 47.78% <11.11%> (-2.27%) ⬇️

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

Impacted Files Coverage Δ
...r/stats/prometheus/PrometheusMetricsGenerator.java 0.00% <0.00%> (-67.40%) ⬇️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 22.86% <0.00%> (ø)
...ersistentStickyKeyDispatcherMultipleConsumers.java 59.31% <25.00%> (-3.07%) ⬇️
...n/java/org/apache/pulsar/broker/admin/v3/Sink.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pulsar/broker/admin/v3/Source.java 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/pulsar/broker/admin/v3/Functions.java 0.00% <0.00%> (-100.00%) ⬇️
...ar/broker/stats/prometheus/ManagedLedgerStats.java 0.00% <0.00%> (-100.00%) ⬇️
...oker/stats/prometheus/PrometheusMetricStreams.java 0.00% <0.00%> (-100.00%) ⬇️
.../stats/prometheus/AggregatedSubscriptionStats.java 0.00% <0.00%> (-100.00%) ⬇️
...metheus/AggregatedTransactionCoordinatorStats.java 0.00% <0.00%> (-100.00%) ⬇️
... and 103 more

@Technoboy- Technoboy- merged commit da87e40 into apache:master Dec 8, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022
…8748)

Co-authored-by: huangzegui <huangzegui@didiglobal.com>
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022
…8748)

Co-authored-by: huangzegui <huangzegui@didiglobal.com>
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
…8748)

Co-authored-by: huangzegui <huangzegui@didiglobal.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants