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 the beforeConsume() method earlier hit with message listener #23578

Merged

Conversation

codelipenghui
Copy link
Contributor

Motivation

This is a fix follow up #23141 which does not cover the earlier hit for beforeConsume() method with message listener. Instead of calling the interceptor before put the message listener task to the message listener thread pool, we should move it to the inside of the message listener task. So that users will not get an earlier hit of the beforeConsume() method in the interceptor.

The expected behavior should be

  • before consume message 0
  • received message 0
  • before consume message 1
  • received message 1
  • before consume message n
  • received message n

Without this fix, the actual behavior was

  • before consumer message 0
  • before consumer message 1
  • before consumer message n
  • received message 0
  • received message 1
  • received message n

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 threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository:

@codelipenghui codelipenghui self-assigned this Nov 7, 2024
@codelipenghui codelipenghui added ready-to-test type/bug The PR fixed a bug or issue reported a bug area/client labels Nov 7, 2024
@codelipenghui codelipenghui added this to the 4.1.0 milestone Nov 7, 2024
@codelipenghui codelipenghui requested a review from coderzc November 7, 2024 19:46
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 7, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.36%. Comparing base (bbc6224) to head (22f5aad).
Report is 721 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23578      +/-   ##
============================================
+ Coverage     73.57%   74.36%   +0.78%     
- Complexity    32624    34951    +2327     
============================================
  Files          1877     1943      +66     
  Lines        139502   147072    +7570     
  Branches      15299    16215     +916     
============================================
+ Hits         102638   109364    +6726     
- Misses        28908    29281     +373     
- Partials       7956     8427     +471     
Flag Coverage Δ
inttests 27.67% <25.00%> (+3.08%) ⬆️
systests 24.36% <75.00%> (+0.03%) ⬆️
unittests 73.72% <100.00%> (+0.88%) ⬆️

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.17% <100.00%> (+1.04%) ⬆️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 80.37% <100.00%> (+2.80%) ⬆️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 77.99% <100.00%> (+0.26%) ⬆️

... and 643 files with indirect coverage changes

@shibd shibd merged commit 137df29 into apache:master Nov 8, 2024
59 of 64 checks passed
@codelipenghui codelipenghui deleted the penghui/fix-before-consume-earlier-hit branch November 8, 2024 03:03
lhotari pushed a commit that referenced this pull request Nov 13, 2024
lhotari pushed a commit that referenced this pull request Nov 13, 2024
lhotari pushed a commit that referenced this pull request Nov 13, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 20, 2024
… listener (apache#23578)

(cherry picked from commit 137df29)
(cherry picked from commit c303cc6)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 21, 2024
… listener (apache#23578)

(cherry picked from commit 137df29)
(cherry picked from commit c303cc6)
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.

7 participants