Skip to content

Conversation

@poorbarcode
Copy link
Contributor

Motivation

Broker checks TTL policies in the background per 5 minutes in a single thread. It calls a block method named topic.isOldestMessageExpired, which causes a prolonged thread blockage.

Modifications

Instead of calling a block method, call a non-blocked method

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added this to the 4.1.0 milestone Aug 5, 2025
@poorbarcode poorbarcode self-assigned this Aug 5, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 5, 2025
Copy link
Member

@dao-jun dao-jun left a comment

Choose a reason for hiding this comment

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

I‘m +1 with the change, but do we need to add tests for the new methods?

@poorbarcode
Copy link
Contributor Author

I‘m +1 with the change, but do we need to add tests for the new methods?

I have added the same test cases that call expireMessages directly. The other case that does not call expireMessages directly should be covered by the old tests, such as testDefaultBacklogTTL

@poorbarcode poorbarcode requested a review from lhotari August 6, 2025 07:25
@poorbarcode poorbarcode requested a review from lhotari August 6, 2025 07:58
@poorbarcode poorbarcode requested a review from lhotari August 7, 2025 02:25
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2025

Codecov Report

❌ Patch coverage is 58.53659% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.32%. Comparing base (d272825) to head (9c8f732).
⚠️ Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
...sar/broker/service/persistent/PersistentTopic.java 52.63% 9 Missing ⚠️
...roker/service/persistent/PersistentReplicator.java 22.22% 6 Missing and 1 partial ⚠️
...rvice/nonpersistent/NonPersistentSubscription.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24606      +/-   ##
============================================
+ Coverage     74.21%   74.32%   +0.11%     
+ Complexity    33142    32789     -353     
============================================
  Files          1881     1881              
  Lines        146770   146814      +44     
  Branches      16859    16862       +3     
============================================
+ Hits         108922   109125     +203     
+ Misses        29181    29022     -159     
  Partials       8667     8667              
Flag Coverage Δ
inttests 26.87% <0.00%> (?)
systests 23.27% <0.00%> (-0.09%) ⬇️
unittests 73.82% <58.53%> (+0.03%) ⬆️

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

Files with missing lines Coverage Δ
...ice/persistent/PersistentMessageExpiryMonitor.java 61.34% <100.00%> (+0.32%) ⬆️
...ker/service/persistent/PersistentSubscription.java 77.50% <100.00%> (+0.98%) ⬆️
...rvice/nonpersistent/NonPersistentSubscription.java 57.64% <0.00%> (-0.26%) ⬇️
...roker/service/persistent/PersistentReplicator.java 72.64% <22.22%> (-2.01%) ⬇️
...sar/broker/service/persistent/PersistentTopic.java 80.05% <52.63%> (-0.37%) ⬇️

... and 108 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.

@Technoboy- Technoboy- merged commit d275bd4 into apache:master Aug 8, 2025
85 of 91 checks passed
gaozhangmin pushed a commit to gaozhangmin/pulsar that referenced this pull request Aug 13, 2025
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Aug 14, 2025
lhotari pushed a commit that referenced this pull request Aug 14, 2025
poorbarcode added a commit to poorbarcode/pulsar that referenced this pull request Aug 14, 2025
Technoboy- pushed a commit that referenced this pull request Aug 18, 2025
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 21, 2025
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 26, 2025
KannarFr pushed a commit to CleverCloud/pulsar that referenced this pull request Sep 22, 2025
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 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.

5 participants