[fix][broker] Wait for in-flight snapshot creation when trimming orphaned delayed-delivery buckets - #26309
Open
lhotari wants to merge 1 commit into
Open
[fix][broker] Wait for in-flight snapshot creation when trimming orphaned delayed-delivery buckets#26309lhotari wants to merge 1 commit into
lhotari wants to merge 1 commit into
Conversation
…aned delayed-delivery buckets A bucket id is only assigned once its snapshot creation completes, so deleting an orphaned bucket while its creation is still in flight makes ImmutableBucket.getAndUpdateBucketId() fall back to a cursor property that has not been written yet and throw NumberFormatException. The exception is thrown synchronously inside the sequential chain built by asyncTrimImmutableBuckets(), which stops at the first failure, so the remaining orphaned buckets are neither deleted from the snapshot storage nor removed from immutableBuckets. deleteBucketSnapshot() now waits for the snapshot creation to settle before deleting, and skips the delete when the creation failed, because afterCreateImmutableBucket() has already removed that bucket and downgraded it to memory mode. Every other snapshot-delete path already awaits the create future; the trim path was the only one missing it. The flaky BucketDelayedDeliveryTrackerTest.testTrimRemovesOrphanedBuckets also awaited only the merging flag, which is satisfied immediately, and then asserted on immutableBuckets while the asynchronous trim was still mutating it. Its assertions now run inside the Awaitility block under the tracker monitor. Assisted-by: Claude Code (Opus 5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
BucketDelayedDeliveryTrackertrims orphaned bucket snapshots — buckets whose ledger range lies entirely before the cursor's mark-delete ledger — whenever sealing a new bucket pushes the bucket count pastdelayedDeliveryMaxNumBuckets. The trim deletes those snapshots without waiting for their snapshot creation to finish.A bucket id is only assigned once its snapshot creation completes. Until then,
ImmutableBucket.getAndUpdateBucketId()falls back to a cursor property that has not been written yet:So deleting a bucket whose creation is still in flight throws
NumberFormatExceptionsynchronously, from inside the sequentialCompletableFuturechain thatasyncTrimImmutableBuckets()builds. That chain deliberately stops at the first failure, so the remaining orphaned buckets are neither deleted from the snapshot storage nor removed fromimmutableBuckets: their snapshots leak, and the buckets stay in the range map until some later trim happens to succeed.Every other snapshot-delete path already handles this —
ImmutableBucket.clear()awaits the create future,asyncMergeBucketSnapshot()awaits the create futures of all merged buckets, and the segment-load path skips a bucket whilegetSnapshotCreateFuture()is not done. The trim path introduced in #25984 was the only one missing it.This surfaced as a flaky
BucketDelayedDeliveryTrackerTest.testTrimRemovesOrphanedBuckets:with the cause visible in the same job's test report:
Two orphaned buckets had been deleted before the chain aborted, which is why the bucket count was already within
maxNumBucketswhile[1..5]was still present.The test additionally had a synchronization gap of its own: it only awaited the
mergingflag, which is satisfied immediately, and then asserted onimmutableBucketswhile the asynchronous trim was still mutating it from a snapshot-storage thread.Modifications
BucketDelayedDeliveryTracker.deleteBucketSnapshot()now waits for the bucket's snapshot creation to settle before deleting it, and skips the delete when the creation failed (INVALID_BUCKET_ID), sinceafterCreateImmutableBucket()has already removed that bucket and downgraded it to memory mode. The existing delete-and-clean-up body moved unchanged into a new privatedoDeleteBucketSnapshot().testTrimRemovesOrphanedBucketsnow performs its assertions inside theAwaitilityblock and reads the range map under the tracker monitor, matching the existingtestMergeSupportsSmallMaxNumBuckets. No assertion was weakened.testTrimWaitsForInFlightSnapshotCreation, which holds all snapshot creations in flight while the trim is triggered and then requires every orphaned bucket to be removed.Verifying this change
This change added tests and can be verified as follows:
BucketDelayedDeliveryTrackerTest.testTrimWaitsForInFlightSnapshotCreationfails deterministically without the production change (Orphaned bucket [1..5] should have been trimmed) and passes with it.BucketDelayedDeliveryTrackerTestis 46/46 green, including 25×invocationCountruns of both trim tests, and theNumberFormatExceptionno longer appears anywhere in the class's output — it was also being logged bytestTrimHandlesDeleteFailure.org.apache.pulsar.broker.delayed.*package (92 tests) and./gradlew quickCheckare green.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes