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 incomingMessageSize and client memory usage is negative #23624

Conversation

TakaHiR07
Copy link
Contributor

@TakaHiR07 TakaHiR07 commented Nov 21, 2024

Fixes #23622

Motivation

After testing for autoScaledReceiverQueueSizeEnabled config, found that the client memory usage and incomingMessageSize is negative.

This error result in two problem:

  1. autoScaledReceiverQueueSize feature is not correct. The receive queue size is expanded based on wrong memory usage.
  2. batchReceivePolicy feature is not correct. The MaxNumBytes config in batchReceivePolicy is judged by wrong incomingMessageSize.

By the way, batchReceivePolicy is default used in multiTopicConsumer since pulsar-2.9.4, while autoScaledReceiverQueueSize is a optional config in consumer.

The reason is because when enter ConsumerImpl#notifyPendingReceivedCallback, the message would directly pass to pendingReceive request. Then consumerImpl just decrease the incomingMessageSize, but not increase. So the value become negative

Modifications

fix the error case that only do decrease the size, but not increase.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

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.

LGTM, great work @TakaHiR07

Copy link
Member

@dao-jun dao-jun left a comment

Choose a reason for hiding this comment

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

Nice catch, good work

@TakaHiR07 TakaHiR07 force-pushed the fix_incomingMessage_and_clientMemory_negative_error branch from 463b90b to 7d79851 Compare November 21, 2024 09:45
@TakaHiR07 TakaHiR07 requested a review from nodece November 21, 2024 09:46
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.35%. Comparing base (bbc6224) to head (7d79851).
Report is 743 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23624      +/-   ##
============================================
+ Coverage     73.57%   74.35%   +0.78%     
- Complexity    32624    34970    +2346     
============================================
  Files          1877     1944      +67     
  Lines        139502   147140    +7638     
  Branches      15299    16224     +925     
============================================
+ Hits         102638   109409    +6771     
- Misses        28908    29282     +374     
- Partials       7956     8449     +493     
Flag Coverage Δ
inttests 27.65% <100.00%> (+3.07%) ⬆️
systests 24.37% <100.00%> (+0.04%) ⬆️
unittests 73.74% <100.00%> (+0.90%) ⬆️

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

Files with missing lines Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerBase.java 75.29% <100.00%> (+1.17%) ⬆️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 80.32% <ø> (+2.75%) ⬆️

... and 656 files with indirect coverage changes

---- 🚨 Try these New Features:

@dao-jun dao-jun merged commit 708c5cc into apache:master Nov 22, 2024
52 checks passed
lhotari pushed a commit that referenced this pull request Nov 22, 2024
…tive (#23624)

Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit 708c5cc)
lhotari pushed a commit that referenced this pull request Nov 22, 2024
…tive (#23624)

Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit 708c5cc)
lhotari pushed a commit that referenced this pull request Nov 22, 2024
…tive (#23624)

Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit 708c5cc)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 26, 2024
…tive (apache#23624)

Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit 708c5cc)
(cherry picked from commit dd07bd6)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 26, 2024
…tive (apache#23624)

Co-authored-by: fanjianye <fanjianye@bigo.sg>
(cherry picked from commit 708c5cc)
(cherry picked from commit dd07bd6)
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.

[Bug][client] Consumer incomingMessageSize and client memory usage is negative
5 participants