[improve][broker]Skip to mark delete if the target position of expira… by poorbarcode · Pull Request #24881 · apache/pulsar

poorbarcode · 2025-10-22T02:49:39Z

Motivation

#24622 improved the message expiration, but forgot to skip expiring if the target position is less than mark deleted position

2025-10-22T02:22:37,301+0000 [BookKeeperClientWorker-OrderedExecutor-2-0] WARN  org.apache.pulsar.broker.service.persistent.PersistentMessageExpiryMonitor - [persistent://t1/ns/tp-partition-8][ostFixtureSubscription2] Message expiry failed - mark delete failed
org.apache.bookkeeper.mledger.ManagedLedgerException: org.apache.bookkeeper.mledger.impl.ManagedCursorImpl$MarkDeletingMarkedPosition: Mark deleting an already mark-deleted position. Current mark-delete: 4200475:458 -- attempted mark delete: 4193092:3606
Caused by: org.apache.bookkeeper.mledger.impl.ManagedCursorImpl$MarkDeletingMarkedPosition: Mark deleting an already mark-deleted position. Current mark-delete: 4200475:458 -- attempted mark delete: 4193092:3606
	at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.setAcknowledgedPosition(ManagedCursorImpl.java:2031) ~[io.streamnative-managed-ledger-4.0.6.6.jar:4.0.6.6]
	at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.asyncMarkDelete(ManagedCursorImpl.java:2163) ~[io.streamnative-managed-ledger-4.0.6.6.jar:4.0.6.6]
	at org.apache.pulsar.broker.service.persistent.PersistentMessageExpiryMonitor.findEntryComplete(PersistentMessageExpiryMonitor.java:237) ~[io.streamnative-pulsar-broker-4.0.6.6.jar:4.0.6.6]
	at org.apache.pulsar.broker.service.persistent.PersistentMessageExpiryMonitor.expireMessages(PersistentMessageExpiryMonitor.java:157) ~[io.streamnative-pulsar-broker-4.0.6.6.jar:4.0.6.6]
	at org.apache.pulsar.broker.service.persistent.PersistentSubscription.expireMessages(PersistentSubscription.java:1258) ~[io.streamnative-pulsar-broker-4.0.6.6.jar:4.0.6.6]
	at org.apache.pulsar.broker.service.persistent.PersistentTopic.lambda$checkMessageExpiryWithSharedPosition$85(PersistentTopic.java:2205) ~[io.streamnative-pulsar-broker-4.0.6.6.jar:4.0.6.6]
	at java.base/java.util.concurrent.ConcurrentHashMap.forEach(Unknown Source) ~[?:?]
	at org.apache.pulsar.broker.service.persistent.PersistentTopic.lambda$checkMessageExpiryWithSharedPosition$88(PersistentTopic.java:2197) ~[io.streamnative-pulsar-broker-4.0.6.6.jar:4.0.6.6]
	at java.base/java.util.concurrent.CompletableFuture$UniAccept.tryFire(Unknown Source) ~[?:?]
	at java.base/java.util.concurrent.CompletableFuture.postComplete(Unknown Source) ~[?:?]
	at java.base/java.util.concurrent.CompletableFuture.complete(Unknown Source) ~[?:?]
	at org.apache.pulsar.broker.service.persistent.PersistentTopic$8.findEntryComplete(PersistentTopic.java:2179) ~[io.streamnative-pulsar-broker-4.0.6.6.jar:4.0.6.6]
	at org.apache.pulsar.broker.service.persistent.PersistentMessageFinder.findEntryComplete(PersistentMessageFinder.java:160) ~[io.streamnative-pulsar-broker-4.0.6.6.jar:4.0.6.6]
	at org.apache.bookkeeper.mledger.impl.OpFindNewest.readEntryComplete(OpFindNewest.java:138) ~[io.streamnative-managed-ledger-4.0.6.6.jar:4.0.6.6]
	at org.apache.bookkeeper.mledger.impl.cache.RangeEntryCacheImpl$1.readEntriesComplete(RangeEntryCacheImpl.java:241) ~[io.streamnative-managed-ledger-4.0.6.6.jar:4.0.6.6]
	at org.apache.bookkeeper.mledger.impl.cache.RangeEntryCacheImpl$2.readEntriesComplete(RangeEntryCacheImpl.java:362) ~[io.streamnative-managed-ledger-4.0.6.6.jar:4.0.6.6]
	at org.apache.bookkeeper.mledger.impl.cache.PendingReadsManager$PendingRead.readEntriesComplete(PendingReadsManager.java:278) ~[io.streamnative-managed-ledger-4.0.6.6.jar:4.0.6.6]
	at org.apache.bookkeeper.mledger.impl.cache.PendingReadsManager$PendingRead.lambda$attach$0(PendingReadsManager.java:244) ~[io.streamnative-managed-ledger-4.0.6.6.jar:4.0.6.6]
	at org.apache.bookkeeper.common.util.SingleThreadExecutor.safeRunTask(SingleThreadExecutor.java:128) ~[io.streamnative-bookkeeper-common-4.17.2.2.jar:4.17.2.2]
	at org.apache.bookkeeper.common.util.SingleThreadExecutor.run(SingleThreadExecutor.java:99) ~[io.streamnative-bookkeeper-common-4.17.2.2.jar:4.17.2.2]
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[io.netty-netty-common-4.1.127.Final.jar:4.1.127.Final]
	at java.base/java.lang.Thread.run(Unknown Source) [?:?]

Modifications

Improve the behaviour

Documentation

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

Matching PR in forked repository

PR in forked repository: x

…tion is less than mark deleted position

Technoboy- · 2025-10-24T08:12:25Z

/pulsarbot rerun-failure-checks

…ersistent/PersistentMessageExpiryMonitor.java Co-authored-by: Zixuan Liu <nodeces@gmail.com>

Technoboy- · 2025-10-27T02:20:42Z

/pulsarbot rerun-failure-checks

#24881) Co-authored-by: Jiwei Guo <jwguo@streamnative.io> Co-authored-by: Zixuan Liu <nodeces@gmail.com>

#24881) Co-authored-by: Jiwei Guo <jwguo@streamnative.io> Co-authored-by: Zixuan Liu <nodeces@gmail.com> (cherry picked from commit 14b0821)

apache#24881) Co-authored-by: Jiwei Guo <jwguo@streamnative.io> Co-authored-by: Zixuan Liu <nodeces@gmail.com> (cherry picked from commit 14b0821) (cherry picked from commit 5bdc5aa)

lhotari · 2025-11-03T16:39:41Z

    public void findEntryComplete(Position position, Object ctx) {
        if (position != null) {
+            var markDeletedPosition = cursor.getMarkDeletedPosition();
+            if (markDeletedPosition != null && markDeletedPosition.compareTo(position) >= 0) {


@poorbarcode This seems to miss expirationCheckInProgress = FALSE;. Could you please check?

The variable will be finally set to false after the task is completely finished(after mark deleting task is finished), see also this two lines:

https://github.com/apache/pulsar/blob/v4.1.1/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java#L217

https://github.com/apache/pulsar/blob/v4.1.1/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java#L226

See more context here: #24607

The variable will be finally set to after the task is completely finished(after mark deleting task is finished), see also this two lines:

Please check again. markDeleteCallback won't get executed at all after the new code in this PR has been added.

I created #24941 to fix the bug and it also contains a test case

lhotari · 2025-11-04T08:26:20Z

There's a follow up in #24941 which fixes a bug introduced by this PR

apache#24881) Co-authored-by: Jiwei Guo <jwguo@streamnative.io> Co-authored-by: Zixuan Liu <nodeces@gmail.com> (cherry picked from commit 14b0821) (cherry picked from commit 5bdc5aa)

[improve][broker]Skip to mark delete if the target position of expira…

a805f66

…tion is less than mark deleted position

poorbarcode added this to the 4.2.0 milestone Oct 22, 2025

poorbarcode self-assigned this Oct 22, 2025

poorbarcode added release/4.1.2 release/4.0.8 ready-to-test labels Oct 22, 2025

poorbarcode closed this Oct 22, 2025

poorbarcode reopened this Oct 22, 2025

Technoboy- approved these changes Oct 22, 2025

View reviewed changes

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Oct 22, 2025

nodece reviewed Oct 22, 2025

View reviewed changes

Comment thread ...rc/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java Outdated

dao-jun approved these changes Oct 23, 2025

View reviewed changes

Update pulsar-broker/src/main/java/org/apache/pulsar/broker/service/p…

b6272ab

…ersistent/PersistentMessageExpiryMonitor.java Co-authored-by: Zixuan Liu <nodeces@gmail.com>

nodece approved these changes Oct 27, 2025

View reviewed changes

Technoboy- merged commit 14b0821 into apache:master Oct 27, 2025
96 of 98 checks passed

Technoboy- added a commit that referenced this pull request Oct 27, 2025

[improve][broker]Skip to mark delete if the target position of expira… (

b7424ac

#24881) Co-authored-by: Jiwei Guo <jwguo@streamnative.io> Co-authored-by: Zixuan Liu <nodeces@gmail.com>

Technoboy- added the cherry-picked/branch-4.1 label Oct 27, 2025

lhotari added the cherry-picked/branch-4.0 label Oct 28, 2025

lhotari reviewed Nov 3, 2025

View reviewed changes

lhotari mentioned this pull request Nov 4, 2025

[fix][broker] Fix bug in PersistentMessageExpiryMonitor which blocked further expirations #24941

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[improve][broker]Skip to mark delete if the target position of expira… - #24881

[improve][broker]Skip to mark delete if the target position of expira…#24881
Technoboy- merged 2 commits into
apache:masterfrom
poorbarcode:improve/skip_expire_msg

poorbarcode commented Oct 22, 2025

Uh oh!

Uh oh!

Technoboy- commented Oct 24, 2025

Uh oh!

Technoboy- commented Oct 27, 2025

Uh oh!

Uh oh!

lhotari Nov 3, 2025

Uh oh!

poorbarcode Nov 3, 2025

Uh oh!

poorbarcode Nov 3, 2025

Uh oh!

lhotari Nov 3, 2025

Uh oh!

lhotari Nov 4, 2025

Uh oh!

poorbarcode Nov 4, 2025

Uh oh!

lhotari commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

poorbarcode commented Oct 22, 2025

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

Uh oh!

Technoboy- commented Oct 24, 2025

Uh oh!

Technoboy- commented Oct 27, 2025

Uh oh!

Uh oh!

lhotari Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

poorbarcode Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

poorbarcode Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

lhotari Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

lhotari Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

poorbarcode Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

lhotari commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants