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

[improve][test] Fix flaky test SimpleProducerConsumerStatTest#testMsgRateExpired #20629

Merged
merged 2 commits into from
Jun 30, 2023

Conversation

massakam
Copy link
Contributor

@massakam massakam commented Jun 21, 2023

Fixes #20615

Motivation

SimpleProducerConsumerStatTest#testMsgRateExpired seems flakey, so I'll try to improve it.

Modifications

Before this fix, we checked if the messages expired by referring to the msgRateExpired value in the subscription stats. However, this value resets to 0 over time, so we instead refer to the totalMsgExpired value, which never resets.

Additionally, we get the stats directly from the instances in the broker server and not from the admin API.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

@massakam massakam added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/test doc-not-needed Your PR changes do not impact docs ready-to-test labels Jun 21, 2023
@massakam massakam added this to the 3.1.0 milestone Jun 21, 2023
@massakam massakam self-assigned this Jun 21, 2023
Awaitility.await().ignoreExceptions().timeout(5, TimeUnit.SECONDS)
.until(() -> admin.topics().getStats(topicName).getSubscriptions().get(subName).getMsgRateExpired() > 0.001);
Awaitility.await().ignoreExceptions().timeout(10, TimeUnit.SECONDS)
.until(() -> pulsar.getBrokerService().getTopicStats().get(topicName).getSubscriptions().get(subName).getTotalMsgExpired() > 0);

Thread.sleep(2000);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this sleep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Technoboy- Removed. Thanks.

@codecov-commenter
Copy link

Codecov Report

Merging #20629 (04261c8) into master (2b01f83) will increase coverage by 39.48%.
The diff coverage is 92.85%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20629       +/-   ##
=============================================
+ Coverage     33.58%   73.07%   +39.48%     
+ Complexity    12127     3701     -8426     
=============================================
  Files          1613     1867      +254     
  Lines        126241   138684    +12443     
  Branches      13770    15240     +1470     
=============================================
+ Hits          42396   101338    +58942     
+ Misses        78331    29310    -49021     
- Partials       5514     8036     +2522     
Flag Coverage Δ
inttests 24.15% <50.00%> (-0.04%) ⬇️
systests 24.97% <50.00%> (?)
unittests 72.36% <92.85%> (+40.32%) ⬆️

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

Impacted Files Coverage Δ
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 78.40% <92.85%> (+46.04%) ⬆️

... and 1520 files with indirect coverage changes

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thank you!

@tisonkun tisonkun merged commit 9526382 into apache:master Jun 30, 2023
@massakam massakam deleted the fix-flaky-test branch June 30, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test doc-not-needed Your PR changes do not impact docs ready-to-test type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: SimpleProducerConsumerStatTest.testMsgRateExpired
5 participants