[fix][broker] Fix snapshot creation race causing delayed delivery bucket trim failures - #26260
[fix][broker] Fix snapshot creation race causing delayed delivery bucket trim failures#26260void-ptr974 wants to merge 1 commit into
Conversation
| } | ||
| } | ||
| return null; | ||
| return bucket.asyncDeleteBucketSnapshot(stats).thenRun(() -> { |
There was a problem hiding this comment.
The orphan eligibility is validated only before the asynchronous snapshot deletion begins. Once this synchronized block is released, the cursor may be reset or its mark‑deleted position could move backward while asyncDeleteBucketSnapshot() is still running.
In such cases, the completion block only rechecks the range‑to‑bucket identity, so it may still delete bucket state even if range.upperEndpoint() < firstActiveLedgerId() no longer holds. The existing test updates firstLedgerId before the snapshot‑create future is released, but it does not cover modifications after this validation and after storage deletion has started.
Could we add a deletion ownership or version guard that spans the duration of the asynchronous operation, or base orphan cleanup on a monotonic physical‑ledger boundary? Please also include a test that blocks deleteBucketSnapshot(), changes firstLedgerId after deletion begins, and then completes the delete.
| .log("Failed to delete bucket snapshot"); | ||
| throw new CompletionException(t); | ||
| } | ||
| return bucket.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE) |
There was a problem hiding this comment.
This ensures the global trimFuture waits for every selected bucket's snapshot-creation future. If any create future remains pending for an extended period, trimFuture.isDone() stays false, which prevents later addMessage() calls from triggering another trim/merge and also chains clear() behind the same future.
Would it be safer to skip buckets still in the CREATING state and retrigger trim when their creation finishes, rather than holding the global trim/merge gate? At minimum, it would be helpful to document the bounded-completion guarantee or add a test for a create future that never completes.
|
@void-ptr974 #26280 should include this fix. When fixing the trim and segement loding, I found this issue. |
Motivation
BucketDelayedDeliveryTrackerpublishes a newly sealed immutable bucket before its asynchronous snapshot creation completes. If the sameaddMessagecall causes the bucket count to exceeddelayedDeliveryMaxNumBuckets, trim can immediately select a bucket whose snapshot ID is not available yet. This makes the current trim pass fail and delays cleanup until a later trim is triggered.Modifications
Consider a tracker with a maximum of 5 buckets and a first active ledger ID of 50:
addMessagecall that creates the sixth bucket starts trim because the limit has been exceeded.getAndUpdateBucketId()callsLong.parseLong(null)and throwsNumberFormatException. The trim chain stops at that failure andtrimFuturecompletes exceptionally. It is not permanently pending, but the failed deletion is left for a later trim attempt.The wait is asynchronous and does not block the dispatcher thread.
Verifying this change
Added deterministic tests that hold snapshot creation in flight and verify that:
Verified with:
./gradlew :pulsar-broker:test --tests org.apache.pulsar.broker.delayed.bucket.BucketDelayedDeliveryTrackerTest./gradlew :pulsar-broker:test --tests org.apache.pulsar.broker.delayed.bucket.BucketDelayedDeliveryTrackerThreadSafetyTest./gradlew quickCheckDoes this pull request potentially affect one of the following parts: