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][test] fix flaky testNegativeAcksWithBackoff when batch enabled. #23986

Merged
merged 3 commits into from
Feb 14, 2025

Conversation

thetumbled
Copy link
Member

@thetumbled thetumbled commented Feb 14, 2025

Main Issue: #23885

Motivation

When batch message is negative ack, some undetermined behavior may occur. For example,

  • There are 10 messages batched into one message, all of them are unbatch into consumer's receiver queue.
  • consumer negative ack the first 5 messages.
  • before the consumer negative ack another 5 messages, the negatve ack tracker trigger the redelivery request.
  • broker dispatch this batched message to consumer.
  • consumer clear the remaining 5 messages in the queue, and refill 10 messages into the queue.
  • Message duplication issue occurs.
    IMO, suck kind of problem is reasonable as we only provide At-Least-Once semantices.

Modifications

This unit test don't handle the batch case at some time.
Negative ack one message is enough for this test in batch case.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

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:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 14, 2025
@lhotari lhotari changed the title [fix] [test] fix flaky testNegativeAcksWithBackoff when batch enabled. [fix][test] fix flaky testNegativeAcksWithBackoff when batch enabled. Feb 14, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.24%. Comparing base (bbc6224) to head (4ab5cc3).
Report is 917 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23986      +/-   ##
============================================
+ Coverage     73.57%   74.24%   +0.66%     
+ Complexity    32624    32265     -359     
============================================
  Files          1877     1853      -24     
  Lines        139502   143847    +4345     
  Branches      15299    16339    +1040     
============================================
+ Hits         102638   106793    +4155     
+ Misses        28908    28648     -260     
- Partials       7956     8406     +450     
Flag Coverage Δ
inttests 26.76% <ø> (+2.18%) ⬆️
systests 23.23% <ø> (-1.09%) ⬇️
unittests 73.76% <ø> (+0.91%) ⬆️

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

see 1042 files with indirect coverage changes

@lhotari lhotari added this to the 4.1.0 milestone Feb 14, 2025
@lhotari lhotari merged commit eb7a4f3 into apache:master Feb 14, 2025
52 checks passed
lhotari pushed a commit that referenced this pull request Feb 17, 2025
lhotari pushed a commit that referenced this pull request Feb 17, 2025
lhotari pushed a commit that referenced this pull request Feb 17, 2025
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Feb 19, 2025
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.

3 participants