Skip to content

[fix][test] flaky test testCanRecoverConsumptionWhenLiftMaxUnAckedMessagesRestriction - #18726

Merged
codelipenghui merged 3 commits into
apache:masterfrom
labuladong:fix-test
Dec 7, 2022
Merged

[fix][test] flaky test testCanRecoverConsumptionWhenLiftMaxUnAckedMessagesRestriction#18726
codelipenghui merged 3 commits into
apache:masterfrom
labuladong:fix-test

Conversation

@labuladong

@labuladong labuladong commented Dec 3, 2022

Copy link
Copy Markdown
Contributor

Fixes #18617

Modifications

The reason for Awaitility timeout:

Producer<byte[]> producer = pulsarClient.newProducer()
.topic(topic)
.enableBatching(true)
.batchingMaxPublishDelay(1, TimeUnit.MILLISECONDS)
// We chose 9 because the maximum unacked message is 10
.batchingMaxMessages(9)
.create();
for (int i = 0; i < totalMsg; i++) {
producer.sendAsync(UUID.randomUUID().toString()
.getBytes(StandardCharsets.UTF_8))
.thenAccept(pubMessages::add);
}

The producer didn't set a key for msg, so even if there are 3 key_shared consumers, there is only 1 consumer to receive messages.

Then this consumer will sync receive and sync ack 1000 messages in 10 - 5 = 5 seconds, a little hurry:

// Wait for all consumers to continue receiving messages.
Awaitility.await()
.pollDelay(5, TimeUnit.SECONDS)
.until(() ->
(System.currentTimeMillis() - lastActiveTime.get()) > TimeUnit.SECONDS.toMillis(5));

Solution:

  1. Construct keys for messages, and ensure all 3 consumers can handle messages.

  2. Give more time to wait for all consumers to handle messages.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

Matching PR in forked repository

PR in forked repository: labuladong#15

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Dec 3, 2022
@codecov-commenter

codecov-commenter commented Dec 3, 2022

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.95%. Comparing base (b36e012) to head (a57c20a).
⚠️ Report is 3198 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18726      +/-   ##
============================================
- Coverage     47.74%   46.95%   -0.79%     
+ Complexity    10623    10552      -71     
============================================
  Files           703      703              
  Lines         68828    68828              
  Branches       7381     7381              
============================================
- Hits          32861    32319     -542     
- Misses        32307    32874     +567     
+ Partials       3660     3635      -25     
Flag Coverage Δ
unittests 46.95% <ø> (-0.79%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 59 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@labuladong

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

1 similar comment
@labuladong

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@labuladong

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

2 similar comments
@labuladong

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@labuladong

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@liangyepianzhou liangyepianzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@labuladong

Copy link
Copy Markdown
Contributor Author

This pr is ready to merge ^_^

@codelipenghui
codelipenghui merged commit 2d205c9 into apache:master Dec 7, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
nodece pushed a commit to nodece/pulsar that referenced this pull request Jan 12, 2024
…ssagesRestriction` (apache#18726)

(cherry picked from commit 2d205c9)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece added a commit to ascentstream/pulsar that referenced this pull request Jan 16, 2024
* [fix][broker] Fix incorrect unack msk count when dup ack a message (apache#20990)

(cherry picked from commit 4facdad)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

* [fix][test] flaky test `testCanRecoverConsumptionWhenLiftMaxUnAckedMessagesRestriction` (apache#18726)

(cherry picked from commit 2d205c9)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

---------

Co-authored-by: Jiwei Guo <technoboy@apache.org>
Co-authored-by: labuladong <labuladong@foxmail.com>
nodece pushed a commit to nodece/pulsar that referenced this pull request Mar 8, 2024
…ssagesRestriction` (apache#18726)

(cherry picked from commit 2d205c9)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece pushed a commit to nodece/pulsar that referenced this pull request Mar 8, 2024
nodece added a commit to ascentstream/pulsar that referenced this pull request Mar 15, 2024
* [fix][broker] Fix incorrect unack msk count when dup ack a message (apache#20990)

(cherry picked from commit 4facdad)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

* [fix][test] flaky test `testCanRecoverConsumptionWhenLiftMaxUnAckedMessagesRestriction` (apache#18726)

(cherry picked from commit 2d205c9)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

---------

Co-authored-by: Jiwei Guo <technoboy@apache.org>
Co-authored-by: labuladong <labuladong@foxmail.com>
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.

Flaky-test: KeySharedSubscriptionTest.testCanRecoverConsumptionWhenLiftMaxUnAckedMessagesRestriction

5 participants