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][broker] Optimize the performance of individual acknowledgments #23072

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

shibd
Copy link
Member

@shibd shibd commented Jul 24, 2024

Motivation

If a subscription has many consumers and each consumer individually acknowledges messages from other consumers, this scenario is very CPU resource-intensive.

Refer this:
image

In fact, these three methods achieve the same goal: get owning consumer by message ID
This operation is very CPU resource-intensive

getAckOwnerConsumer:

private Consumer getAckOwnerConsumer(long ledgerId, long entryId) {
Consumer ackOwnerConsumer = this;
if (Subscription.isIndividualAckMode(subType)) {
if (!getPendingAcks().containsKey(ledgerId, entryId)) {
for (Consumer consumer : subscription.getConsumers()) {
if (consumer != this && consumer.getPendingAcks().containsKey(ledgerId, entryId)) {
ackOwnerConsumer = consumer;
break;
}
}
}
}
return ackOwnerConsumer;
}

getBatchSize:

Consumer ackOwnerConsumer = getAckOwnerConsumer(msgId.getLedgerId(), msgId.getEntryId());

removePendingAcks:

if (pendingAcks.get(position.getLedgerId(), position.getEntryId()) == null) {
for (Consumer consumer : subscription.getConsumers()) {
if (!consumer.equals(this) && consumer.getPendingAcks().containsKey(position.getLedgerId(),
position.getEntryId())) {
ackOwnedConsumer = consumer;
break;
}
}
} else {
ackOwnedConsumer = this;
}

Modifications

This PR ensures that the getAckOwnerConsumer method is called only once during the processing of a message ack. In the future, we may need to optimize this method further.

After this PR:
image

ATT: Client Side Test Code: https://gist.github.com/shibd/82b77eccc8145ac2c1784aca97a29c24

Verifying this change

All tests passed to make sure not have any break change.

Documentation

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

@shibd shibd self-assigned this Jul 24, 2024
@shibd shibd added ready-to-test category/performance Performance issues fix or improvements labels Jul 24, 2024
@shibd shibd added this to the 3.4.0 milestone Jul 24, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 24, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 91.30435% with 4 lines in your changes missing coverage. Please review.

Project coverage is 73.44%. Comparing base (bbc6224) to head (679ac08).
Report is 474 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23072      +/-   ##
============================================
- Coverage     73.57%   73.44%   -0.13%     
- Complexity    32624    33503     +879     
============================================
  Files          1877     1917      +40     
  Lines        139502   144051    +4549     
  Branches      15299    15734     +435     
============================================
+ Hits         102638   105803    +3165     
- Misses        28908    30127    +1219     
- Partials       7956     8121     +165     
Flag Coverage Δ
inttests 27.49% <67.39%> (+2.90%) ⬆️
systests 24.75% <41.30%> (+0.43%) ⬆️
unittests 72.50% <91.30%> (-0.34%) ⬇️

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

Files Coverage Δ
...ava/org/apache/pulsar/broker/service/Consumer.java 86.65% <91.30%> (-0.02%) ⬇️

... and 511 files with indirect coverage changes

@shibd shibd requested a review from gaoran10 July 25, 2024 07:58
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Great catch @shibd . Added a suggestion about renaming a method & variable since the meaning has changed.

@shibd shibd merged commit 77b6378 into apache:master Jul 29, 2024
55 of 56 checks passed
shibd added a commit that referenced this pull request Jul 30, 2024
shibd added a commit that referenced this pull request Jul 30, 2024
shibd added a commit that referenced this pull request Jul 30, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 30, 2024
…nts (apache#23072)

(cherry picked from commit 77b6378)
(cherry picked from commit 487913e)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 30, 2024
…nts (apache#23072)

(cherry picked from commit 77b6378)
(cherry picked from commit 487913e)
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.

4 participants