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][broker] Fix closing of AbstractDispatcherSingleActiveConsumer and reduce flakiness of test #21736

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Dec 15, 2023

Fixes #21756

Motivation & Modifications

Closing of AbstractDispatcherSingleActiveConsumer should also close the underlying rate limiter. This will leaks resources in rate limiters that hold on resources (before PIP-322 changes, thus relevant to maintenance branches). The SubscriptionMessageDispatchThrottlingTest consistently fails since closing is missing.

In addition to this, reduce flakiness in SubscriptionMessageDispatchThrottlingTest.
Often the timeouts are too short. There shouldn't be a need to have strict 500 ms timeouts in tests. Remove Awaitility "atMost" timeouts and rely on default (10 secs). Change all test timeouts to 30 seconds in the class.

Additional context

It looks like PR #21682 changes broke closing in certain cases. That problem is now fixed in this PR.
This impacted PersistentDispatcherSingleActiveConsumer.close(boolean disconnectClients, Optional<BrokerLookupData> assignedBrokerLookupData) method call which is implemented in AbstractDispatcherSingleActiveConsumer . That method didn't close the rate limiter and it's now included in this PR.

Documentation

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

@lhotari lhotari self-assigned this Dec 15, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 15, 2023
@lhotari lhotari changed the title [fix][test] Fix flaky SubscriptionMessageDispatchThrottlingTest [fix][test] Reduce flakiness in SubscriptionMessageDispatchThrottlingTest Dec 17, 2023
@lhotari lhotari changed the title [fix][test] Reduce flakiness in SubscriptionMessageDispatchThrottlingTest [fix][broker] Fix closing of AbstractDispatcherSingleActiveConsumer and reduce flakiness of test Dec 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2023

Codecov Report

Merging #21736 (22e016c) into master (631b13a) will increase coverage by 43.08%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #21736       +/-   ##
=============================================
+ Coverage     30.33%   73.41%   +43.08%     
- Complexity      326    32785    +32459     
=============================================
  Files          1719     1897      +178     
  Lines        131059   140622     +9563     
  Branches      14320    15486     +1166     
=============================================
+ Hits          39759   103242    +63483     
+ Misses        85315    29310    -56005     
- Partials       5985     8070     +2085     
Flag Coverage Δ
inttests 24.17% <75.00%> (-0.07%) ⬇️
systests 24.76% <75.00%> (-0.12%) ⬇️
unittests 72.71% <100.00%> (?)

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

Files Coverage Δ
...ervice/AbstractDispatcherSingleActiveConsumer.java 90.55% <100.00%> (+23.88%) ⬆️
...a/org/apache/pulsar/broker/service/Dispatcher.java 55.55% <100.00%> (+18.05%) ⬆️
...tent/NonPersistentDispatcherMultipleConsumers.java 68.67% <100.00%> (+68.67%) ⬆️
...sistent/PersistentDispatcherMultipleConsumers.java 75.54% <ø> (+36.45%) ⬆️
...tent/PersistentDispatcherSingleActiveConsumer.java 70.93% <ø> (+23.08%) ⬆️

... and 1516 files with indirect coverage changes

@lhotari lhotari force-pushed the lh-fix-SubscriptionMessageDispatchThrottlingTest branch from 460470e to bfe63e4 Compare December 19, 2023 10:54
@Technoboy- Technoboy- added this to the 3.2.0 milestone Dec 19, 2023
@lhotari lhotari merged commit ed68ec1 into apache:master Dec 19, 2023
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test type/bug The PR fixed a bug or issue reported a bug type/flaky-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: SubscriptionMessageDispatchThrottlingTest.testClosingRateLimiter
3 participants