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.

Sign up for GitHub

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

Jump to bottom

[fix][client] Consumer lost message ack due to race condition in acknowledge with batch message #22353

Merged
merged 8 commits into from
Mar 27, 2024
Merged

[fix][client] Consumer lost message ack due to race condition in acknowledge with batch message #22353

merged 8 commits into from
Mar 27, 2024

Conversation

Shawyeok
Copy link
Contributor

@Shawyeok Shawyeok commented Mar 26, 2024 •
edited
Loading

Motivation

Fixes #22352

Modifications

Add synchronize protection on bitset operations acknowledge with batch message

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added unit test for MessageIdAdvUtilsTest#acknowledge in multi-thread scenario

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: Shawyeok#15

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 26, 2024
@Shawyeok Shawyeok mentioned this pull request Mar 26, 2024
Closed
3 tasks
@nodece nodece requested a review from BewareMyPower March 26, 2024 04:12
BewareMyPower
BewareMyPower previously approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

It would be better to update the API documentation as well. We must note that any read or modification on the return value of MessageIdAdv#getAckSet() should be synchronized on the lock as well.

@BewareMyPower BewareMyPower dismissed their stale review March 26, 2024 09:58

It should be comments rather than approved

lhotari
lhotari reviewed Mar 26, 2024
View reviewed changes
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.

I have some questions about the test case.

lhotari
lhotari approved these changes Mar 26, 2024
View reviewed changes
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.

Good work @Shawyeok

@Shawyeok
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@lhotari lhotari added the type/bug The PR fixed a bug or issue reported a bug label Mar 27, 2024
@lhotari lhotari merged commit 3fa2ae8 into apache:master Mar 27, 2024
51 of 52 checks passed
@Shawyeok
Copy link
Contributor Author

Thank you for review and suggestions. @lhotari @BewareMyPower @poorbarcode

lhotari pushed a commit that referenced this pull request Mar 27, 2024
@Shawyeok @lhotari
[fix][client] Consumer lost message ack due to race condition in ackn…
c01f72e 
…owledge with batch message (#22353)

Co-authored-by: Yunze Xu <[email protected]>
Co-authored-by: 汪苏诚 <[email protected]>
(cherry picked from commit 3fa2ae8)
lhotari pushed a commit that referenced this pull request Mar 27, 2024
@Shawyeok @lhotari
[fix][client] Consumer lost message ack due to race condition in ackn…
9c50d18 
…owledge with batch message (#22353)

Co-authored-by: Yunze Xu <[email protected]>
Co-authored-by: 汪苏诚 <[email protected]>
(cherry picked from commit 3fa2ae8)
@Shawyeok Shawyeok deleted the fix/message-ack-lost branch March 28, 2024 07:43
@Technoboy- Technoboy- added this to the 3.3.0 milestone Apr 1, 2024
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Apr 1, 2024
@Shawyeok @BewareMyPower @Technoboy-
[fix][client] Consumer lost message ack due to race condition in ackn…
33b742a 
…owledge with batch message (apache#22353)

Co-authored-by: Yunze Xu <[email protected]>
Co-authored-by: 汪苏诚 <[email protected]>
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 15, 2024
@Shawyeok @mukesh-ctds
[fix][client] Consumer lost message ack due to race condition in ackn…
8c31dfa 
…owledge with batch message (apache#22353)

Co-authored-by: Yunze Xu <[email protected]>
Co-authored-by: 汪苏诚 <[email protected]>
(cherry picked from commit 3fa2ae8)
(cherry picked from commit 9c50d18)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
@Shawyeok @mukesh-ctds
[fix][client] Consumer lost message ack due to race condition in ackn…
e284a02 
…owledge with batch message (apache#22353)

Co-authored-by: Yunze Xu <[email protected]>
Co-authored-by: 汪苏诚 <[email protected]>
(cherry picked from commit 3fa2ae8)
(cherry picked from commit 9c50d18)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
@Shawyeok @mukesh-ctds
[fix][client] Consumer lost message ack due to race condition in ackn…
7fdde10 
…owledge with batch message (apache#22353)

Co-authored-by: Yunze Xu <[email protected]>
Co-authored-by: 汪苏诚 <[email protected]>
(cherry picked from commit 3fa2ae8)
(cherry picked from commit 9c50d18)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 19, 2024
@Shawyeok @mukesh-ctds
[fix][client] Consumer lost message ack due to race condition in ackn…
ba52a49 
…owledge with batch message (apache#22353)

Co-authored-by: Yunze Xu <[email protected]>
Co-authored-by: 汪苏诚 <[email protected]>
(cherry picked from commit 3fa2ae8)
(cherry picked from commit 9c50d18)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
@Shawyeok @srinath-ctds
[fix][client] Consumer lost message ack due to race condition in ackn…
3233fd2 
…owledge with batch message (apache#22353)

Co-authored-by: Yunze Xu <[email protected]>
Co-authored-by: 汪苏诚 <[email protected]>
(cherry picked from commit 3fa2ae8)
(cherry picked from commit 9c50d18)
@lhotari lhotari mentioned this pull request Sep 3, 2024
Open
2 tasks
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
@Shawyeok @BewareMyPower @hanmz
[fix][client] Consumer lost message ack due to race condition in ackn…
c90d291 
…owledge with batch message (apache#22353)

Co-authored-by: Yunze Xu <[email protected]>
Co-authored-by: 汪苏诚 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhotari lhotari lhotari approved these changes

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@Demogorgon314 Demogorgon314 Awaiting requested review from Demogorgon314

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

Assignees

@Shawyeok Shawyeok

Labels
cherry-picked/branch-3.0 cherry-picked/branch-3.2 doc-not-needed Your PR changes do not impact docs release/3.0.4 release/3.1.4 release/3.2.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.

[Bug][Client] Consumer lost message ack due to race condition in acknowledge with batch message
5 participants
@Shawyeok @lhotari @BewareMyPower @poorbarcode @Technoboy-