-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[bug][cpp] Fix issue where unexpected ack timeout occurred #17503
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
massakam
added
type/bug
The PR fixed a bug or issue reported a bug
component/client-c++
labels
Sep 7, 2022
BewareMyPower
requested review from
Demogorgon314,
k2la,
merlimat,
rdhabalia,
BewareMyPower and
RobertIndie
September 7, 2022 13:36
BewareMyPower
approved these changes
Sep 7, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice catch!
github-actions
bot
added
doc-label-missing
and removed
doc-not-needed
Your PR changes do not impact docs
labels
Sep 7, 2022
@massakam Please provide a correct documentation label for your PR. |
github-actions
bot
added
doc-not-needed
Your PR changes do not impact docs
and removed
doc-label-missing
labels
Sep 8, 2022
massakam
force-pushed
the
fix-cpp-ack-timeout
branch
from
September 9, 2022 01:38
a60d5ca
to
7888a34
Compare
lhotari
approved these changes
Sep 9, 2022
shibd
approved these changes
Sep 13, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
congbobo184
pushed a commit
that referenced
this pull request
Nov 14, 2022
(cherry picked from commit a98f025)
congbobo184
pushed a commit
that referenced
this pull request
Dec 7, 2022
(cherry picked from commit a98f025)
congbobo184
added a commit
that referenced
this pull request
Dec 7, 2022
14 tasks
liangyepianzhou
added a commit
that referenced
this pull request
Dec 14, 2022
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
Jan 10, 2023
…ache#18906) ### Motivation Cherry-pick apache#17503 to release 2.10.3 and run tests. ### Modifications Cherry-pick apache#17503 to release 2.10.3. (cherry picked from commit b5cfde1)
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
Jan 11, 2023
…ache#18906) ### Motivation Cherry-pick apache#17503 to release 2.10.3 and run tests. ### Modifications Cherry-pick apache#17503 to release 2.10.3. (cherry picked from commit b5cfde1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
cherry-picked/branch-2.9
Archived: 2.9 is end of life
cherry-picked/branch-2.10
doc-not-needed
Your PR changes do not impact docs
release/2.8.5
release/2.9.4
release/2.10.3
type/bug
The PR fixed a bug or issue reported a bug
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
When we start a consumer like the following and publish multiple messages to a topic, ack timeout occurs even though the messages received are immediately acknowledged. This does not occur with normal topics, only with partitioned topics.
The reason is that the task of executing the message listener function is queued to
listenerExecutor_
immediately after adding the message tounAckedMessageTrackerPtr_
, but it is not always executed immediately. If it takes a long time to execute each message listener function like the code for reproduction above, it will take a long time for the queued tasks to actually execute, causing an ack timeout. I don't think this is the expected behavior.pulsar/pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc
Lines 462 to 464 in 0bbc4e1
Modifications
Have
listenerExecutor_
perform adding messages tounAckedMessageTrackerPtr_
. In this way, the message listener function will always be executed immediately after adding a message tounAckedMessageTrackerPtr_
, preventing unexpected ack timeouts.Verifying this change
Documentation
doc-not-needed