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

[client] Optimize pause when creating sub consumers in multi-topic consumer #14566

Merged
merged 1 commit into from
Mar 23, 2022

Conversation

Jason918
Copy link
Contributor

@Jason918 Jason918 commented Mar 5, 2022

Motivation

Currently, if we subscribe new partitions/topics when multiTopicConsumer is paused, the sub consumers will start in non-pause mode and then changed to pause.

Modifications

Set startPause config when creating sub consumers.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as

  • org.apache.pulsar.client.api.PartitionedProducerConsumerTest#testPauseAndResume
  • org.apache.pulsar.client.api.SimpleProducerConsumerTest#testPauseAndResume
  • org.apache.pulsar.client.impl.ZeroQueueSizeTest#testPauseAndResume

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc

Internal optimization.

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 5, 2022
@Jason918
Copy link
Contributor Author

Jason918 commented Mar 6, 2022

/pulsarbot run-failure-checks

2 similar comments
@Jason918
Copy link
Contributor Author

Jason918 commented Mar 7, 2022

/pulsarbot run-failure-checks

@Jason918
Copy link
Contributor Author

Jason918 commented Mar 7, 2022

/pulsarbot run-failure-checks

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

LGTM, is it able to add a test?

@codelipenghui codelipenghui added this to the 2.11.0 milestone Mar 15, 2022
@aloyszhang
Copy link
Contributor

aloyszhang commented Mar 15, 2022

@Jason918
After creating a new ConsumerImpl for a topic partition, before sending FLOW command, client will check

if (!(firstTimeConnect && hasParentConsumer) && getCurrentReceiverQueueSize() != 0) {
                    increaseAvailablePermits(cnx, getCurrentReceiverQueueSize());
                }

hasParentConsumer is always true for multiTopicConsumer, so the new created ConsumerImpl will not send FLOW command to broker.

BTW, #11974 introduce startPaused for creating new ConsumerImpl, so this pull request is a nice optimize for me.

Left a comment, PTAL.

@Jason918
Copy link
Contributor Author

After creating a new ConsumerImpl for a topic partition, before sending FLOW command, client will check

if (!(firstTimeConnect && hasParentConsumer) && getCurrentReceiverQueueSize() != 0) {
                    increaseAvailablePermits(cnx, getCurrentReceiverQueueSize());
                }

hasParentConsumer is always true for multiTopicConsumer, so the new created ConsumerImpl will not send FLOW command to broker.

Great point. I have updated the motivation description.

@Jason918 Jason918 merged commit a32edc7 into apache:master Mar 23, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
congbobo184 pushed a commit that referenced this pull request Nov 26, 2022
(cherry picked from commit a32edc7)
@congbobo184 congbobo184 added cherry-picked/branch-2.9 Archived: 2.9 is end of life release/2.9.4 labels Nov 26, 2022
liangyepianzhou pushed a commit that referenced this pull request Jun 29, 2023
(cherry picked from commit a32edc7)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 3, 2023
(cherry picked from commit a32edc7)
(cherry picked from commit 90369a0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants