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] Reduce flakiness of AdminApi2Test #20529

Merged
merged 2 commits into from Jun 7, 2023

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jun 7, 2023

Fixes #20376

Motivation

AdminApi2Test is extremely flaky and causes most builds to fail.

Modifications

  • calling resetConfig method between test methods is the wrong solution since it creates a new instance and the running broker will keep using the old instance
    • reset properties on the existing conf instance
  • use unique tenants and namespaces to make it easier to track where the possibly conflicting topics were created, if the problems preserve
  • add a better solution for resetting state between test methods when it is needed
  • catch exceptions in deletion and restart the cluster in the case of exceptions
    • this will handle the flakiness with resetClusters.
    • The tradeoff is that it could hide some problems that the flakiness is revealing.

Documentation

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

@lhotari lhotari added this to the 3.1.0 milestone Jun 7, 2023
@lhotari lhotari self-assigned this Jun 7, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 7, 2023
@lhotari
Copy link
Member Author

lhotari commented Jun 7, 2023

/pulsarbot rerun-failure-checks

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

Also this test class is very big, maybe it would be worth splitting it into smaller classes

- calling resetConfig method between test methods is the wrong solution
- use unique tenants and namespaces to make it easier to track where
  the possibly conflicting topics were created, if the problems
  preserve
@lhotari lhotari force-pushed the lh-fix-flaky-AdminApi2Test branch from faf1d9a to 81eb7d0 Compare June 7, 2023 14:02
@lhotari
Copy link
Member Author

lhotari commented Jun 7, 2023

Also this test class is very big, maybe it would be worth splitting it into smaller classes

+1

@codecov-commenter
Copy link

Codecov Report

Merging #20529 (089a84b) into master (d7186a6) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20529      +/-   ##
============================================
- Coverage     72.93%   72.90%   -0.04%     
+ Complexity    31930    31820     -110     
============================================
  Files          1867     1867              
  Lines        138555   138554       -1     
  Branches      15218    15218              
============================================
- Hits         101059   101015      -44     
- Misses        29466    29522      +56     
+ Partials       8030     8017      -13     
Flag Coverage Δ
inttests 24.20% <100.00%> (-0.04%) ⬇️
systests 24.91% <100.00%> (+0.01%) ⬆️
unittests 72.19% <100.00%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
...rg/apache/pulsar/broker/service/BrokerService.java 80.69% <ø> (+0.14%) ⬆️
...sar/broker/service/persistent/PersistentTopic.java 79.39% <100.00%> (ø)

... and 75 files with indirect coverage changes

@lhotari lhotari merged commit 60dba5d into apache:master Jun 7, 2023
42 of 43 checks passed
lhotari added a commit that referenced this pull request Jun 7, 2023
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: AdminApi2Test.resetClusters
4 participants