Skip to content

Conversation

@codelipenghui
Copy link
Contributor

Motivation

The resetCursor(long timestamp) method was not protected by the subscription fencing mechanism (IS_FENCED_UPDATER), allowing concurrent reset operations to interfere with one another. This may result in performance degradation during timestamp-based position lookups, particularly when tiered storage is enabled.

Modifications

Apply the same fencing mechanism used by resetCursor(Position) to resetCursor(long timestamp) by sharing the internal resetCursorInternal() method.

Verifying this change

  • Added unit test testConcurrentResetCursorByTimestamp in SubscriptionSeekTest.java
  • Run: mvn -pl pulsar-broker -Dtest=SubscriptionSeekTest#testConcurrentResetCursorByTimestamp test

Documentation

  • doc-not-needed

codelipenghui added a commit to codelipenghui/incubator-pulsar that referenced this pull request Jan 16, 2026
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 16, 2026
@codelipenghui codelipenghui self-assigned this Jan 16, 2026
@codelipenghui codelipenghui added this to the 4.2.0 milestone Jan 16, 2026
@codelipenghui codelipenghui changed the title [fix][broker] Fence reset cursor by timestamp to avoid concurrent resets [fix][broker] Fence reset cursor by timestamp to avoid concurrent timestamp-based position lookups Jan 16, 2026
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 Report

❌ Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.66%. Comparing base (99fdca8) to head (70b15f4).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...ker/service/persistent/PersistentSubscription.java 69.23% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #25151       +/-   ##
=============================================
+ Coverage     37.41%   72.66%   +35.25%     
- Complexity    13148    33884    +20736     
=============================================
  Files          1899     1956       +57     
  Lines        150564   154736     +4172     
  Branches      17156    17643      +487     
=============================================
+ Hits          56334   112440    +56106     
+ Misses        86500    33345    -53155     
- Partials       7730     8951     +1221     
Flag Coverage Δ
inttests 25.78% <15.38%> (+0.13%) ⬆️
systests 22.51% <15.38%> (+0.05%) ⬆️
unittests 73.61% <69.23%> (+39.57%) ⬆️

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

Files with missing lines Coverage Δ
...ker/service/persistent/PersistentSubscription.java 75.78% <69.23%> (+29.10%) ⬆️

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

@lhotari lhotari merged commit 6a3066d into apache:master Jan 16, 2026
148 of 153 checks passed
lhotari pushed a commit that referenced this pull request Jan 16, 2026
…estamp-based position lookups (#25151)

(cherry picked from commit 6a3066d)
lhotari pushed a commit that referenced this pull request Jan 16, 2026
…estamp-based position lookups (#25151)

(cherry picked from commit 6a3066d)
@codelipenghui codelipenghui deleted the fix-fence-reset-cursor-timestamp branch January 16, 2026 15:44
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.

3 participants