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 publish callback's entry data is null during ledger rollover #10467

Merged

Conversation

BewareMyPower
Copy link
Contributor

Motivation

#9257 added an extra ByteBuf parameter to managed ledger's AddEntryCallback#addComplete so that broker can get a buffer view of the entry when it's persisted successfully. However, when a ledger is full and the rollover is triggered, the addComplete method will be triggered in OpAddEntry#closeComplete and the argument is null.

Modifications

  • Extend the lifetime of data when ledger.asyncClose is called in OpAddEntry#safeRun, and pass the data to cb.addComplete, after that, release the data.
  • Make current test cover the ledger rollover case.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as ManagedLedgerTest#asyncAddEntryWithoutError.

@BewareMyPower
Copy link
Contributor Author

/pulsarbot run-failure-checks

@merlimat merlimat added the type/bug The PR fixed a bug or issue reported a bug label May 3, 2021
@merlimat merlimat added this to the 2.8.0 milestone May 3, 2021
@merlimat merlimat merged commit 7296274 into apache:master May 3, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/npe-close-ledger branch May 4, 2021 09:10
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request May 11, 2021
…che#10467)

* Fix publish callback's entry data is null during ledger rollover

* Don't call recycle() when the callback is null
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants