Skip to content

Conversation

@BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Sep 15, 2025

Main Issue: #24697

Motivation

Using callback-based read API with a Netty recyclable object is buggy that once the ReadEntriesCtx object has been recycled twice.

Modifications

  • Add a future-based API wrapper on asyncReadEntriesWithSkipOrWait
  • Add testConcurrentRead to verify concurrent read will fail
  • Modify the CompactedTopicUtils#asyncReadCompactedEntries API to return a future as well
  • Remove ReadEntriesCtx

In extreme cases like #24697, it's safe to deliver messages to the consumer after that.

Documentation

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

Matching PR in forked repository

PR in forked repository:

@BewareMyPower BewareMyPower self-assigned this Sep 15, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 15, 2025
@BewareMyPower
Copy link
Contributor Author

While the reason of #24697 is still in investigation, this PR aims at the hard-to-debug recycling issue that did happen in real world by eliminating the recycler in read path

Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

@BewareMyPower BewareMyPower merged commit ccbd245 into apache:master Sep 18, 2025
97 of 102 checks passed
@BewareMyPower BewareMyPower deleted the bewaremypower/read-future-based-api branch September 18, 2025 03:13
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, great job @BewareMyPower.

lhotari pushed a commit that referenced this pull request Sep 18, 2025
lhotari pushed a commit that referenced this pull request Sep 18, 2025
lhotari pushed a commit that referenced this pull request Sep 18, 2025
@lhotari
Copy link
Member

lhotari commented Sep 18, 2025

Cherry-picking to branch-3.0 causes merge conflicts mainly due to #22285 being missing in branch-3.0, perhaps others. It was fairly easy to backport to branch-3.3. @BewareMyPower can you please handle cherry-picking to branch-3.0? I already resolved other merge conflicts in branch-3.3 commit 65ddd44 so you should be able to cherry-pick that to branch-3.0 once the preconditions in branch-3.0 have been addressed.

@BewareMyPower
Copy link
Contributor Author

Okay, I will do that

ganesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Sep 18, 2025
… callback (apache#24741)

(cherry picked from commit ccbd245)
(cherry picked from commit 215a1fd)
BewareMyPower added a commit that referenced this pull request Sep 18, 2025
@BewareMyPower
Copy link
Contributor Author

@lhotari I've completed the cherry-picking, as well as cherry-picking #20522 before this PR

srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Sep 18, 2025
… callback (apache#24741)

(cherry picked from commit ccbd245)
(cherry picked from commit 215a1fd)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Sep 22, 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.

4 participants