From cb78263120cbe2247cf355d63acd49c36538f0a2 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Fri, 15 Aug 2025 21:26:23 +0800 Subject: [PATCH 01/16] feat(delayed): optimize BucketDelayedDeliveryTracker with new lock strategy Refactor lock mechanism from StampedLock to ReentrantReadWriteLock for thread safety. Add async bucket snapshot creation and improve concurrent message handling. --- .../bucket/BucketDelayedDeliveryTracker.java | 205 ++++++++++++------ 1 file changed, 133 insertions(+), 72 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index 3f0fcc516571f..823eec457c83f 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -40,10 +40,14 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; -import java.util.concurrent.locks.StampedLock; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.stream.Collectors; import javax.annotation.concurrent.ThreadSafe; import lombok.Getter; @@ -59,6 +63,7 @@ import org.apache.pulsar.broker.delayed.proto.DelayedIndex; import org.apache.pulsar.broker.delayed.proto.SnapshotSegment; import org.apache.pulsar.broker.service.persistent.AbstractPersistentDispatcherMultipleConsumers; +import org.apache.pulsar.client.util.ExecutorProvider; import org.apache.pulsar.common.policies.data.stats.TopicMetricBean; import org.apache.pulsar.common.util.FutureUtil; import org.apache.pulsar.common.util.collections.TripleLongPriorityQueue; @@ -94,12 +99,9 @@ public static record SnapshotKey(long ledgerId, long entryId) {} private final AtomicLong numberDelayedMessages = new AtomicLong(0); - // Thread safety locks - private final StampedLock stampedLock = new StampedLock(); - @Getter @VisibleForTesting - private final MutableBucket lastMutableBucket; + private volatile MutableBucket lastMutableBucket; @Getter @VisibleForTesting @@ -115,6 +117,13 @@ public static record SnapshotKey(long ledgerId, long entryId) {} private CompletableFuture pendingLoad = null; + private final ExecutorService bucketSnapshotExecutor; + private final AtomicBoolean bucketSnapshotInProgress = new AtomicBoolean(false); + private volatile MutableBucket bucketBeingSealed = null; + + private final Lock readLock; + private final Lock writeLock; + public BucketDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumers dispatcher, Timer timer, long tickTimeMillis, boolean isDelayedDeliveryDeliverAtTimeStrict, @@ -146,6 +155,11 @@ public BucketDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumer new MutableBucket(dispatcher.getName(), dispatcher.getCursor(), FutureUtil.Sequencer.create(), bucketSnapshotStorage); this.stats = new BucketDelayedMessageIndexStats(); + ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock(); + this.readLock = rwLock.readLock(); + this.writeLock = rwLock.writeLock(); + bucketSnapshotExecutor = Executors.newSingleThreadScheduledExecutor( + new ExecutorProvider.ExtendedThreadFactory("bucket-creation")); // Close the tracker if failed to recover. try { @@ -365,56 +379,82 @@ private void afterCreateImmutableBucket(Pair immu } @Override - public synchronized boolean addMessage(long ledgerId, long entryId, long deliverAt) { - if (containsMessage(ledgerId, entryId)) { - return true; - } + public boolean addMessage(long ledgerId, long entryId, long deliverAt) { + readLock.lock(); + try { + if (containsMessageUnsafe(ledgerId, entryId)) { + return true; + } - if (deliverAt < 0 || deliverAt <= getCutoffTime()) { - return false; + if (deliverAt < 0 || deliverAt <= getCutoffTime()) { + return false; + } + } finally { + readLock.unlock(); } - boolean existBucket = findImmutableBucket(ledgerId).isPresent(); - - // Create bucket snapshot - if (!existBucket && ledgerId > lastMutableBucket.endLedgerId - && lastMutableBucket.size() >= minIndexCountPerBucket - && !lastMutableBucket.isEmpty()) { - long createStartTime = System.currentTimeMillis(); - stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.create); - Pair immutableBucketDelayedIndexPair = - lastMutableBucket.sealBucketAndAsyncPersistent( - this.timeStepPerBucketSnapshotSegmentInMillis, - this.maxIndexesPerBucketSnapshotSegment, - this.sharedBucketPriorityQueue); - afterCreateImmutableBucket(immutableBucketDelayedIndexPair, createStartTime); - lastMutableBucket.resetLastMutableBucketRange(); + writeLock.lock(); + try { + // Double check + if (containsMessageUnsafe(ledgerId, entryId)) { + return true; + } - if (maxNumBuckets > 0 && immutableBuckets.asMapOfRanges().size() > maxNumBuckets) { - asyncMergeBucketSnapshot(); + if (deliverAt <= getCutoffTime()) { + return false; } - } - if (ledgerId < lastMutableBucket.startLedgerId || existBucket) { - // If (ledgerId < startLedgerId || existBucket) means that message index belong to previous bucket range, - // enter sharedBucketPriorityQueue directly - sharedBucketPriorityQueue.add(deliverAt, ledgerId, entryId); - lastMutableBucket.putIndexBit(ledgerId, entryId); - } else { - checkArgument(ledgerId >= lastMutableBucket.endLedgerId); - lastMutableBucket.addMessage(ledgerId, entryId, deliverAt); - } + final MutableBucket sealingBucket = this.bucketBeingSealed; + if (sealingBucket != null + && ledgerId >= sealingBucket.startLedgerId + && ledgerId <= sealingBucket.endLedgerId) { + sharedBucketPriorityQueue.add(deliverAt, ledgerId, entryId); + sealingBucket.putIndexBit(ledgerId, entryId); + } else { + boolean existBucket = findImmutableBucket(ledgerId).isPresent(); + + if (!existBucket + && ledgerId > lastMutableBucket.endLedgerId + && lastMutableBucket.size() >= minIndexCountPerBucket + && !lastMutableBucket.isEmpty() + && bucketSnapshotInProgress.compareAndSet(false, true)) { + // Create bucket snapshot + this.bucketBeingSealed = this.lastMutableBucket; + this.lastMutableBucket = new MutableBucket(dispatcher.getName(), dispatcher.getCursor(), + FutureUtil.Sequencer.create(), this.lastMutableBucket.getBucketSnapshotStorage()); + bucketSnapshotExecutor.execute(() -> { + try { + createBucketSnapshotAsync(); + } finally { + bucketSnapshotInProgress.set(false); + } + }); + } - numberDelayedMessages.incrementAndGet(); + if (ledgerId < lastMutableBucket.startLedgerId || existBucket) { + // If (ledgerId < startLedgerId || existBucket) + // means that message index belong to previous bucket range, + // enter sharedBucketPriorityQueue directly + sharedBucketPriorityQueue.add(deliverAt, ledgerId, entryId); + lastMutableBucket.putIndexBit(ledgerId, entryId); + } else { + checkArgument(ledgerId >= lastMutableBucket.endLedgerId); + lastMutableBucket.addMessage(ledgerId, entryId, deliverAt); + } + } - if (log.isDebugEnabled()) { - log.debug("[{}] Add message {}:{} -- Delivery in {} ms ", dispatcher.getName(), ledgerId, entryId, - deliverAt - clock.millis()); - } + numberDelayedMessages.incrementAndGet(); - updateTimer(); + if (log.isDebugEnabled()) { + log.debug("[{}] Add message {}:{} -- Delivery in {} ms ", dispatcher.getName(), ledgerId, entryId, + deliverAt - clock.millis()); + } - return true; + updateTimer(); + return true; + } finally { + writeLock.unlock(); + } } private synchronized List selectMergedBuckets(final List values, int mergeNum) { @@ -565,6 +605,44 @@ private synchronized CompletableFuture asyncMergeBucketSnapshot(List immutableBucketDelayedIndexPair = + bucketToSeal.sealBucketAndAsyncPersistent( + this.timeStepPerBucketSnapshotSegmentInMillis, + this.maxIndexesPerBucketSnapshotSegment, + this.sharedBucketPriorityQueue); + if (immutableBucketDelayedIndexPair == null) { + return; + } + + writeLock.lock(); + try { + afterCreateImmutableBucket(immutableBucketDelayedIndexPair, createStartTime); + if (this.bucketBeingSealed == bucketToSeal) { + this.bucketBeingSealed = null; + } + + if (maxNumBuckets > 0 && immutableBuckets.asMapOfRanges().size() > maxNumBuckets) { + asyncMergeBucketSnapshot(); + } + } finally { + writeLock.unlock(); + } + } finally { + if (this.bucketBeingSealed == bucketToSeal) { + this.bucketBeingSealed = null; + } + } + } + @Override public synchronized boolean hasMessageAvailable() { long cutoffTime = getCutoffTime(); @@ -578,20 +656,12 @@ public synchronized boolean hasMessageAvailable() { @Override protected long nextDeliveryTime() { - // Use optimistic read for frequently called method - long stamp = stampedLock.tryOptimisticRead(); - long result = nextDeliveryTimeUnsafe(); - - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = nextDeliveryTimeUnsafe(); - } finally { - stampedLock.unlockRead(stamp); - } + readLock.lock(); + try { + return nextDeliveryTimeUnsafe(); + } finally { + readLock.unlock(); } - return result; } private long nextDeliveryTimeUnsafe() { @@ -789,21 +859,12 @@ private boolean removeIndexBit(long ledgerId, long entryId) { } public boolean containsMessage(long ledgerId, long entryId) { - // Try optimistic read first for best performance - long stamp = stampedLock.tryOptimisticRead(); - boolean result = containsMessageUnsafe(ledgerId, entryId); - - - if (!stampedLock.validate(stamp)) { - // Fall back to read lock if validation fails - stamp = stampedLock.readLock(); - try { - result = containsMessageUnsafe(ledgerId, entryId); - } finally { - stampedLock.unlockRead(stamp); - } + readLock.lock(); + try { + return containsMessageUnsafe(ledgerId, entryId); + } finally { + readLock.unlock(); } - return result; } private boolean containsMessageUnsafe(long ledgerId, long entryId) { From ffbe45b7ca64ea95cf4ce17da0174c6aece95bc9 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Mon, 25 Aug 2025 21:50:30 +0800 Subject: [PATCH 02/16] refactor(bucket): replace synchronized with explicit write lock in asyncMergeBucketSnapshot --- .../bucket/BucketDelayedDeliveryTracker.java | 191 +++++++++--------- 1 file changed, 101 insertions(+), 90 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index 823eec457c83f..0e0551f925af4 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -457,7 +457,7 @@ public boolean addMessage(long ledgerId, long entryId, long deliverAt) { } } - private synchronized List selectMergedBuckets(final List values, int mergeNum) { + private List selectMergedBuckets(final List values, int mergeNum) { checkArgument(mergeNum < values.size()); long minNumberMessages = Long.MAX_VALUE; long minScheduleTimestamp = Long.MAX_VALUE; @@ -494,115 +494,126 @@ private synchronized List selectMergedBuckets(final List asyncMergeBucketSnapshot() { - List immutableBucketList = immutableBuckets.asMapOfRanges().values().stream().toList(); - List toBeMergeImmutableBuckets = selectMergedBuckets(immutableBucketList, MAX_MERGE_NUM); + private CompletableFuture asyncMergeBucketSnapshot() { + writeLock.lock(); + try { + List immutableBucketList = immutableBuckets.asMapOfRanges().values().stream().toList(); + List toBeMergeImmutableBuckets = selectMergedBuckets(immutableBucketList, MAX_MERGE_NUM); - if (toBeMergeImmutableBuckets.isEmpty()) { - log.warn("[{}] Can't find able merged buckets", dispatcher.getName()); - return CompletableFuture.completedFuture(null); - } + if (toBeMergeImmutableBuckets.isEmpty()) { + log.warn("[{}] Can't find able merged buckets", dispatcher.getName()); + return CompletableFuture.completedFuture(null); + } - final String bucketsStr = toBeMergeImmutableBuckets.stream().map(Bucket::bucketKey).collect( - Collectors.joining(",")).replaceAll(DELAYED_BUCKET_KEY_PREFIX + "_", ""); - if (log.isDebugEnabled()) { - log.info("[{}] Merging bucket snapshot, bucketKeys: {}", dispatcher.getName(), bucketsStr); - } + final String bucketsStr = toBeMergeImmutableBuckets.stream().map(Bucket::bucketKey).collect( + Collectors.joining(",")).replaceAll(DELAYED_BUCKET_KEY_PREFIX + "_", ""); + if (log.isDebugEnabled()) { + log.info("[{}] Merging bucket snapshot, bucketKeys: {}", dispatcher.getName(), bucketsStr); + } - for (ImmutableBucket immutableBucket : toBeMergeImmutableBuckets) { - immutableBucket.merging = true; - } + for (ImmutableBucket immutableBucket : toBeMergeImmutableBuckets) { + immutableBucket.merging = true; + } - long mergeStartTime = System.currentTimeMillis(); - stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.merge); - return asyncMergeBucketSnapshot(toBeMergeImmutableBuckets).whenComplete((__, ex) -> { - synchronized (this) { - for (ImmutableBucket immutableBucket : toBeMergeImmutableBuckets) { - immutableBucket.merging = false; + long mergeStartTime = System.currentTimeMillis(); + stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.merge); + return asyncMergeBucketSnapshot(toBeMergeImmutableBuckets).whenComplete((__, ex) -> { + writeLock.lock(); + try { + for (ImmutableBucket immutableBucket : toBeMergeImmutableBuckets) { + immutableBucket.merging = false; + } + } finally { + writeLock.unlock(); } - } - if (ex != null) { - log.error("[{}] Failed to merge bucket snapshot, bucketKeys: {}", - dispatcher.getName(), bucketsStr, ex); + if (ex != null) { + log.error("[{}] Failed to merge bucket snapshot, bucketKeys: {}", + dispatcher.getName(), bucketsStr, ex); - stats.recordFailEvent(BucketDelayedMessageIndexStats.Type.merge); - } else { - log.info("[{}] Merge bucket snapshot finish, bucketKeys: {}, bucketNum: {}", - dispatcher.getName(), bucketsStr, immutableBuckets.asMapOfRanges().size()); + stats.recordFailEvent(BucketDelayedMessageIndexStats.Type.merge); + } else { + log.info("[{}] Merge bucket snapshot finish, bucketKeys: {}, bucketNum: {}", + dispatcher.getName(), bucketsStr, immutableBuckets.asMapOfRanges().size()); - stats.recordSuccessEvent(BucketDelayedMessageIndexStats.Type.merge, - System.currentTimeMillis() - mergeStartTime); - } - }); + stats.recordSuccessEvent(BucketDelayedMessageIndexStats.Type.merge, + System.currentTimeMillis() - mergeStartTime); + } + }); + } finally { + writeLock.unlock(); + } } - private synchronized CompletableFuture asyncMergeBucketSnapshot(List buckets) { + private CompletableFuture asyncMergeBucketSnapshot(List buckets) { List> createFutures = buckets.stream().map(bucket -> bucket.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE)) .toList(); - return FutureUtil.waitForAll(createFutures).thenCompose(bucketId -> { - if (createFutures.stream().anyMatch(future -> INVALID_BUCKET_ID.equals(future.join()))) { - return FutureUtil.failedFuture(new RuntimeException("Can't merge buckets due to bucket create failed")); - } + return FutureUtil.waitForAll(createFutures).thenCompose(bucketId -> { + if (createFutures.stream().anyMatch(future -> INVALID_BUCKET_ID.equals(future.join()))) { + return FutureUtil.failedFuture(new RuntimeException("Can't merge buckets due to bucket create failed")); + } - List>> getRemainFutures = - buckets.stream().map(ImmutableBucket::getRemainSnapshotSegment).toList(); - - return FutureUtil.waitForAll(getRemainFutures) - .thenApply(__ -> { - return CombinedSegmentDelayedIndexQueue.wrap( - getRemainFutures.stream().map(CompletableFuture::join).toList()); - }) - .thenAccept(combinedDelayedIndexQueue -> { - synchronized (BucketDelayedDeliveryTracker.this) { - long createStartTime = System.currentTimeMillis(); - stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.create); - Pair immutableBucketDelayedIndexPair = - lastMutableBucket.createImmutableBucketAndAsyncPersistent( - timeStepPerBucketSnapshotSegmentInMillis, - maxIndexesPerBucketSnapshotSegment, - sharedBucketPriorityQueue, combinedDelayedIndexQueue, - buckets.get(0).startLedgerId, - buckets.get(buckets.size() - 1).endLedgerId); - - // Merge bit map to new bucket - Map delayedIndexBitMap = - new HashMap<>(buckets.get(0).getDelayedIndexBitMap()); - for (int i = 1; i < buckets.size(); i++) { - buckets.get(i).delayedIndexBitMap.forEach((ledgerId, bitMapB) -> { - delayedIndexBitMap.compute(ledgerId, (k, bitMap) -> { - if (bitMap == null) { - return bitMapB; - } - - bitMap.or(bitMapB); - return bitMap; + List>> getRemainFutures = + buckets.stream().map(ImmutableBucket::getRemainSnapshotSegment).toList(); + + return FutureUtil.waitForAll(getRemainFutures) + .thenApply(__ -> { + return CombinedSegmentDelayedIndexQueue.wrap( + getRemainFutures.stream().map(CompletableFuture::join).toList()); + }) + .thenAccept(combinedDelayedIndexQueue -> { + writeLock.lock(); + try { + long createStartTime = System.currentTimeMillis(); + stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.create); + Pair immutableBucketDelayedIndexPair = + lastMutableBucket.createImmutableBucketAndAsyncPersistent( + timeStepPerBucketSnapshotSegmentInMillis, + maxIndexesPerBucketSnapshotSegment, + sharedBucketPriorityQueue, combinedDelayedIndexQueue, + buckets.get(0).startLedgerId, + buckets.get(buckets.size() - 1).endLedgerId); + + // Merge bit map to new bucket + Map delayedIndexBitMap = + new HashMap<>(buckets.get(0).getDelayedIndexBitMap()); + for (int i = 1; i < buckets.size(); i++) { + buckets.get(i).delayedIndexBitMap.forEach((ledgerId, bitMapB) -> { + delayedIndexBitMap.compute(ledgerId, (k, bitMap) -> { + if (bitMap == null) { + return bitMapB; + } + + bitMap.or(bitMapB); + return bitMap; + }); }); - }); - } + } - // optimize bm - delayedIndexBitMap.values().forEach(RoaringBitmap::runOptimize); - immutableBucketDelayedIndexPair.getLeft().setDelayedIndexBitMap(delayedIndexBitMap); + // optimize bm + delayedIndexBitMap.values().forEach(RoaringBitmap::runOptimize); + immutableBucketDelayedIndexPair.getLeft().setDelayedIndexBitMap(delayedIndexBitMap); - afterCreateImmutableBucket(immutableBucketDelayedIndexPair, createStartTime); + afterCreateImmutableBucket(immutableBucketDelayedIndexPair, createStartTime); - immutableBucketDelayedIndexPair.getLeft().getSnapshotCreateFuture() - .orElse(NULL_LONG_PROMISE).thenCompose(___ -> { - List> removeFutures = - buckets.stream().map(bucket -> bucket.asyncDeleteBucketSnapshot(stats)) - .toList(); - return FutureUtil.waitForAll(removeFutures); - }); + immutableBucketDelayedIndexPair.getLeft().getSnapshotCreateFuture() + .orElse(NULL_LONG_PROMISE).thenCompose(___ -> { + List> removeFutures = + buckets.stream().map(bucket -> bucket.asyncDeleteBucketSnapshot(stats)) + .toList(); + return FutureUtil.waitForAll(removeFutures); + }); - for (ImmutableBucket bucket : buckets) { - immutableBuckets.asMapOfRanges() - .remove(Range.closed(bucket.startLedgerId, bucket.endLedgerId)); + for (ImmutableBucket bucket : buckets) { + immutableBuckets.asMapOfRanges() + .remove(Range.closed(bucket.startLedgerId, bucket.endLedgerId)); + } + } finally { + writeLock.unlock(); } - } - }); - }); + }); + }); } private void createBucketSnapshotAsync() { From 4e8b23ed723f27d29100fa914400e4237791fbbd Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Wed, 17 Sep 2025 20:40:10 +0800 Subject: [PATCH 03/16] fix(broker): adjust indentation in BucketDelayedDeliveryTracker --- .../bucket/BucketDelayedDeliveryTracker.java | 116 +++++++++--------- 1 file changed, 58 insertions(+), 58 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index 0e0551f925af4..f2fa7adcc8229 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -549,71 +549,71 @@ private CompletableFuture asyncMergeBucketSnapshot(List b buckets.stream().map(bucket -> bucket.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE)) .toList(); - return FutureUtil.waitForAll(createFutures).thenCompose(bucketId -> { - if (createFutures.stream().anyMatch(future -> INVALID_BUCKET_ID.equals(future.join()))) { - return FutureUtil.failedFuture(new RuntimeException("Can't merge buckets due to bucket create failed")); - } + return FutureUtil.waitForAll(createFutures).thenCompose(bucketId -> { + if (createFutures.stream().anyMatch(future -> INVALID_BUCKET_ID.equals(future.join()))) { + return FutureUtil.failedFuture(new RuntimeException("Can't merge buckets due to bucket create failed")); + } - List>> getRemainFutures = - buckets.stream().map(ImmutableBucket::getRemainSnapshotSegment).toList(); - - return FutureUtil.waitForAll(getRemainFutures) - .thenApply(__ -> { - return CombinedSegmentDelayedIndexQueue.wrap( - getRemainFutures.stream().map(CompletableFuture::join).toList()); - }) - .thenAccept(combinedDelayedIndexQueue -> { - writeLock.lock(); - try { - long createStartTime = System.currentTimeMillis(); - stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.create); - Pair immutableBucketDelayedIndexPair = - lastMutableBucket.createImmutableBucketAndAsyncPersistent( - timeStepPerBucketSnapshotSegmentInMillis, - maxIndexesPerBucketSnapshotSegment, - sharedBucketPriorityQueue, combinedDelayedIndexQueue, - buckets.get(0).startLedgerId, - buckets.get(buckets.size() - 1).endLedgerId); - - // Merge bit map to new bucket - Map delayedIndexBitMap = - new HashMap<>(buckets.get(0).getDelayedIndexBitMap()); - for (int i = 1; i < buckets.size(); i++) { - buckets.get(i).delayedIndexBitMap.forEach((ledgerId, bitMapB) -> { - delayedIndexBitMap.compute(ledgerId, (k, bitMap) -> { - if (bitMap == null) { - return bitMapB; - } - - bitMap.or(bitMapB); - return bitMap; - }); + List>> getRemainFutures = + buckets.stream().map(ImmutableBucket::getRemainSnapshotSegment).toList(); + + return FutureUtil.waitForAll(getRemainFutures) + .thenApply(__ -> { + return CombinedSegmentDelayedIndexQueue.wrap( + getRemainFutures.stream().map(CompletableFuture::join).toList()); + }) + .thenAccept(combinedDelayedIndexQueue -> { + writeLock.lock(); + try { + long createStartTime = System.currentTimeMillis(); + stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.create); + Pair immutableBucketDelayedIndexPair = + lastMutableBucket.createImmutableBucketAndAsyncPersistent( + timeStepPerBucketSnapshotSegmentInMillis, + maxIndexesPerBucketSnapshotSegment, + sharedBucketPriorityQueue, combinedDelayedIndexQueue, + buckets.get(0).startLedgerId, + buckets.get(buckets.size() - 1).endLedgerId); + + // Merge bit map to new bucket + Map delayedIndexBitMap = + new HashMap<>(buckets.get(0).getDelayedIndexBitMap()); + for (int i = 1; i < buckets.size(); i++) { + buckets.get(i).delayedIndexBitMap.forEach((ledgerId, bitMapB) -> { + delayedIndexBitMap.compute(ledgerId, (k, bitMap) -> { + if (bitMap == null) { + return bitMapB; + } + + bitMap.or(bitMapB); + return bitMap; }); - } + }); + } - // optimize bm - delayedIndexBitMap.values().forEach(RoaringBitmap::runOptimize); - immutableBucketDelayedIndexPair.getLeft().setDelayedIndexBitMap(delayedIndexBitMap); + // optimize bm + delayedIndexBitMap.values().forEach(RoaringBitmap::runOptimize); + immutableBucketDelayedIndexPair.getLeft().setDelayedIndexBitMap(delayedIndexBitMap); - afterCreateImmutableBucket(immutableBucketDelayedIndexPair, createStartTime); + afterCreateImmutableBucket(immutableBucketDelayedIndexPair, createStartTime); - immutableBucketDelayedIndexPair.getLeft().getSnapshotCreateFuture() - .orElse(NULL_LONG_PROMISE).thenCompose(___ -> { - List> removeFutures = - buckets.stream().map(bucket -> bucket.asyncDeleteBucketSnapshot(stats)) - .toList(); - return FutureUtil.waitForAll(removeFutures); - }); + immutableBucketDelayedIndexPair.getLeft().getSnapshotCreateFuture() + .orElse(NULL_LONG_PROMISE).thenCompose(___ -> { + List> removeFutures = + buckets.stream().map(bucket -> bucket.asyncDeleteBucketSnapshot(stats)) + .toList(); + return FutureUtil.waitForAll(removeFutures); + }); - for (ImmutableBucket bucket : buckets) { - immutableBuckets.asMapOfRanges() - .remove(Range.closed(bucket.startLedgerId, bucket.endLedgerId)); - } - } finally { - writeLock.unlock(); + for (ImmutableBucket bucket : buckets) { + immutableBuckets.asMapOfRanges() + .remove(Range.closed(bucket.startLedgerId, bucket.endLedgerId)); } - }); - }); + } finally { + writeLock.unlock(); + } + }); + }); } private void createBucketSnapshotAsync() { From 00fa1e371587a0a7e7c9f6c577db285aa1a96932 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Wed, 17 Sep 2025 21:39:21 +0800 Subject: [PATCH 04/16] refactor(delayed-delivery): replace synchronized with writeLock in BucketDelayedDeliveryTracker --- .../bucket/BucketDelayedDeliveryTracker.java | 69 ++++++++++++------- 1 file changed, 45 insertions(+), 24 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index f2fa7adcc8229..9b7ffc56ed594 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -306,11 +306,14 @@ private synchronized void putAndCleanOverlapRange(Range range, ImmutableBu @Override public void run(Timeout timeout) throws Exception { - synchronized (this) { + writeLock.lock(); + try { if (timeout == null || timeout.isCancelled()) { return; } lastMutableBucket.moveScheduledMessageToSharedQueue(getCutoffTime(), sharedBucketPriorityQueue); + } finally { + writeLock.unlock(); } super.run(timeout); } @@ -354,7 +357,8 @@ private void afterCreateImmutableBucket(Pair immu stats.recordFailEvent(BucketDelayedMessageIndexStats.Type.create); // Put indexes back into the shared queue and downgrade to memory mode - synchronized (BucketDelayedDeliveryTracker.this) { + writeLock.lock(); + try { immutableBucket.getSnapshotSegments().ifPresent(snapshotSegments -> { for (SnapshotSegment snapshotSegment : snapshotSegments) { for (DelayedIndex delayedIndex : snapshotSegment.getIndexesList()) { @@ -370,6 +374,8 @@ private void afterCreateImmutableBucket(Pair immu Range.closed(immutableBucket.startLedgerId, immutableBucket.endLedgerId)); snapshotSegmentLastIndexMap.remove( new SnapshotKey(lastDelayedIndex.getLedgerId(), lastDelayedIndex.getEntryId())); + } finally { + writeLock.unlock(); } return INVALID_BUCKET_ID; }); @@ -655,14 +661,19 @@ private void createBucketSnapshotAsync() { } @Override - public synchronized boolean hasMessageAvailable() { - long cutoffTime = getCutoffTime(); + public boolean hasMessageAvailable() { + writeLock.lock(); + try { + long cutoffTime = getCutoffTime(); - boolean hasMessageAvailable = getNumberOfDelayedMessages() > 0 && nextDeliveryTime() <= cutoffTime; - if (!hasMessageAvailable) { - updateTimer(); + boolean hasMessageAvailable = getNumberOfDelayedMessages() > 0 && nextDeliveryTimeUnsafe() <= cutoffTime; + if (!hasMessageAvailable) { + updateTimer(); + } + return hasMessageAvailable; + } finally { + writeLock.unlock(); } - return hasMessageAvailable; } @Override @@ -825,26 +836,36 @@ public boolean shouldPauseAllDeliveries() { } @Override - public synchronized CompletableFuture clear() { - CompletableFuture future = cleanImmutableBuckets(); - sharedBucketPriorityQueue.clear(); - lastMutableBucket.clear(); - snapshotSegmentLastIndexMap.clear(); - numberDelayedMessages.set(0); - return future; + public CompletableFuture clear() { + writeLock.lock(); + try { + CompletableFuture future = cleanImmutableBuckets(); + sharedBucketPriorityQueue.clear(); + lastMutableBucket.clear(); + snapshotSegmentLastIndexMap.clear(); + numberDelayedMessages.set(0); + return future; + } finally { + writeLock.unlock(); + } } @Override - public synchronized void close() { - super.close(); - lastMutableBucket.close(); - sharedBucketPriorityQueue.close(); + public void close() { + writeLock.lock(); try { - List> completableFutures = immutableBuckets.asMapOfRanges().values().stream() - .map(bucket -> bucket.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE)).toList(); - FutureUtil.waitForAll(completableFutures).get(AsyncOperationTimeoutSeconds, TimeUnit.SECONDS); - } catch (Exception e) { - log.warn("[{}] Failed wait to snapshot generate", dispatcher.getName(), e); + super.close(); + lastMutableBucket.close(); + sharedBucketPriorityQueue.close(); + try { + List> completableFutures = immutableBuckets.asMapOfRanges().values().stream() + .map(bucket -> bucket.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE)).toList(); + FutureUtil.waitForAll(completableFutures).get(AsyncOperationTimeoutSeconds, TimeUnit.SECONDS); + } catch (Exception e) { + log.warn("[{}] Failed wait to snapshot generate", dispatcher.getName(), e); + } + } finally { + writeLock.unlock(); } } From 47280a96e9682103d279d6cbd8412d1232bd6e49 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Fri, 19 Sep 2025 17:33:20 +0800 Subject: [PATCH 05/16] refactor(broker): Optimize the mixed read-write operations in getScheduledMessages --- .../bucket/BucketDelayedDeliveryTracker.java | 140 +++++++++--------- 1 file changed, 67 insertions(+), 73 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index 9b7ffc56ed594..4f99eb5785306 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -708,8 +708,8 @@ public long getBufferMemoryUsage() { } @Override - public synchronized NavigableSet getScheduledMessages(int maxMessages) { - if (!checkPendingLoadDone()) { + public NavigableSet getScheduledMessages(int maxMessages) { + if (pendingLoad != null && !pendingLoad.isDone()) { if (log.isDebugEnabled()) { log.debug("[{}] Skip getScheduledMessages to wait for bucket snapshot load finish.", dispatcher.getName()); @@ -719,115 +719,109 @@ public synchronized NavigableSet getScheduledMessages(int maxMessages) long cutoffTime = getCutoffTime(); - lastMutableBucket.moveScheduledMessageToSharedQueue(cutoffTime, sharedBucketPriorityQueue); - NavigableSet positions = new TreeSet<>(); - int n = maxMessages; - - while (n > 0 && !sharedBucketPriorityQueue.isEmpty()) { - long timestamp = sharedBucketPriorityQueue.peekN1(); - if (timestamp > cutoffTime) { - break; - } - - long ledgerId = sharedBucketPriorityQueue.peekN2(); - long entryId = sharedBucketPriorityQueue.peekN3(); - - SnapshotKey snapshotKey = new SnapshotKey(ledgerId, entryId); - - ImmutableBucket bucket = snapshotSegmentLastIndexMap.get(snapshotKey); - if (bucket != null && immutableBuckets.asMapOfRanges().containsValue(bucket)) { - // All message of current snapshot segment are scheduled, try load next snapshot segment - if (bucket.merging) { - log.info("[{}] Skip load to wait for bucket snapshot merge finish, bucketKey:{}", - dispatcher.getName(), bucket.bucketKey()); + writeLock.lock(); + try { + lastMutableBucket.moveScheduledMessageToSharedQueue(cutoffTime, sharedBucketPriorityQueue); + while (positions.size() < maxMessages && !sharedBucketPriorityQueue.isEmpty()) { + if (sharedBucketPriorityQueue.peekN1() > cutoffTime) { + // All remaining messages are scheduled for the future break; } - - final int preSegmentEntryId = bucket.currentSegmentEntryId; - if (log.isDebugEnabled()) { - log.debug("[{}] Loading next bucket snapshot segment, bucketKey: {}, nextSegmentEntryId: {}", - dispatcher.getName(), bucket.bucketKey(), preSegmentEntryId + 1); - } - boolean createFutureDone = bucket.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE).isDone(); - if (!createFutureDone) { - log.info("[{}] Skip load to wait for bucket snapshot create finish, bucketKey:{}", - dispatcher.getName(), bucket.bucketKey()); + long ledgerId = sharedBucketPriorityQueue.peekN2(); + long entryId = sharedBucketPriorityQueue.peekN3(); + // Check if this message is a trigger to load the next snapshot segment + SnapshotKey snapshotKey = new SnapshotKey(ledgerId, entryId); + ImmutableBucket bucket = snapshotSegmentLastIndexMap.get(snapshotKey); + if (bucket != null && immutableBuckets.asMapOfRanges().containsValue(bucket)) { + // This is the last message of a segment. We need to load the next one. + // Trigger the load and stop processing more messages in this run. + // The positions collected so far will be returned. + triggerAsyncLoadBucketSnapshot(bucket, snapshotKey); break; } + // If it's a regular message (or in memory-only mode), process it. + sharedBucketPriorityQueue.pop(); // Consume the message from the queue + positions.add(PositionFactory.create(ledgerId, entryId)); + removeIndexBit(ledgerId, entryId); + } + if (!positions.isEmpty()) { + numberDelayedMessages.addAndGet(-positions.size()); + updateTimer(); + } + } finally { + writeLock.unlock(); + } + + return positions; + } + + private void triggerAsyncLoadBucketSnapshot(ImmutableBucket bucketToLoad, SnapshotKey snapshotKeyToLoad) { + final int preSegmentEntryId = bucketToLoad.currentSegmentEntryId; + if (log.isDebugEnabled()) { + log.debug("[{}] Loading next bucket snapshot segment, bucketKey: {}, nextSegmentEntryId: {}", + dispatcher.getName(), bucketToLoad.bucketKey(), preSegmentEntryId + 1); + } + + boolean createFutureDone = bucketToLoad.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE).isDone(); + if (!createFutureDone) { + log.info("[{}] Skip load to wait for bucket snapshot create finish, bucketKey:{}", + dispatcher.getName(), bucketToLoad.bucketKey()); + return; + } - long loadStartTime = System.currentTimeMillis(); - stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.load); - CompletableFuture loadFuture = pendingLoad = bucket.asyncLoadNextBucketSnapshotEntry() - .thenAccept(indexList -> { - synchronized (BucketDelayedDeliveryTracker.this) { - this.snapshotSegmentLastIndexMap.remove(snapshotKey); + long loadStartTime = System.currentTimeMillis(); + stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.load); + CompletableFuture loadFuture = pendingLoad = bucketToLoad.asyncLoadNextBucketSnapshotEntry() + .thenAccept(indexList -> { + writeLock.lock(); + try { + this.snapshotSegmentLastIndexMap.remove(snapshotKeyToLoad); if (CollectionUtils.isEmpty(indexList)) { immutableBuckets.asMapOfRanges() - .remove(Range.closed(bucket.startLedgerId, bucket.endLedgerId)); - bucket.asyncDeleteBucketSnapshot(stats); + .remove(Range.closed(bucketToLoad.startLedgerId, bucketToLoad.endLedgerId)); + bucketToLoad.asyncDeleteBucketSnapshot(stats); return; } DelayedIndex lastDelayedIndex = indexList.get(indexList.size() - 1); this.snapshotSegmentLastIndexMap.put( new SnapshotKey(lastDelayedIndex.getLedgerId(), lastDelayedIndex.getEntryId()), - bucket); + bucketToLoad); for (DelayedIndex index : indexList) { sharedBucketPriorityQueue.add(index.getTimestamp(), index.getLedgerId(), index.getEntryId()); } + } finally { + writeLock.unlock(); } }).whenComplete((__, ex) -> { if (ex != null) { // Back bucket state - bucket.setCurrentSegmentEntryId(preSegmentEntryId); + bucketToLoad.setCurrentSegmentEntryId(preSegmentEntryId); log.error("[{}] Failed to load bucket snapshot segment, bucketKey: {}, segmentEntryId: {}", - dispatcher.getName(), bucket.bucketKey(), preSegmentEntryId + 1, ex); + dispatcher.getName(), bucketToLoad.bucketKey(), preSegmentEntryId + 1, ex); stats.recordFailEvent(BucketDelayedMessageIndexStats.Type.load); } else { log.info("[{}] Load next bucket snapshot segment finish, bucketKey: {}, segmentEntryId: {}", - dispatcher.getName(), bucket.bucketKey(), - (preSegmentEntryId == bucket.lastSegmentEntryId) ? "-1" : preSegmentEntryId + 1); + dispatcher.getName(), bucketToLoad.bucketKey(), + (preSegmentEntryId == bucketToLoad.lastSegmentEntryId) ? "-1" : preSegmentEntryId + 1); stats.recordSuccessEvent(BucketDelayedMessageIndexStats.Type.load, System.currentTimeMillis() - loadStartTime); } - synchronized (this) { + writeLock.lock(); + try { if (timeout != null) { timeout.cancel(); } timeout = timer.newTimeout(this, 0, TimeUnit.MILLISECONDS); + } finally { + writeLock.unlock(); } }); - - if (!checkPendingLoadDone() || loadFuture.isCompletedExceptionally()) { - break; - } - } - - positions.add(PositionFactory.create(ledgerId, entryId)); - - sharedBucketPriorityQueue.pop(); - removeIndexBit(ledgerId, entryId); - - --n; - numberDelayedMessages.decrementAndGet(); - } - - updateTimer(); - - return positions; - } - - private synchronized boolean checkPendingLoadDone() { - if (pendingLoad == null || pendingLoad.isDone()) { - pendingLoad = null; - return true; - } - return false; } @Override From 2542478acceea606906db2a99e8b9da96178247f Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Fri, 19 Sep 2025 20:42:12 +0800 Subject: [PATCH 06/16] refactor(broker): improve delayed delivery tracker close logic --- .../bucket/BucketDelayedDeliveryTracker.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index 4f99eb5785306..e2adedae90069 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -846,21 +846,30 @@ public CompletableFuture clear() { @Override public void close() { + List> completableFutures = Collections.emptyList(); writeLock.lock(); try { super.close(); lastMutableBucket.close(); sharedBucketPriorityQueue.close(); try { - List> completableFutures = immutableBuckets.asMapOfRanges().values().stream() + completableFutures = immutableBuckets.asMapOfRanges().values().stream() .map(bucket -> bucket.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE)).toList(); - FutureUtil.waitForAll(completableFutures).get(AsyncOperationTimeoutSeconds, TimeUnit.SECONDS); } catch (Exception e) { log.warn("[{}] Failed wait to snapshot generate", dispatcher.getName(), e); } } finally { writeLock.unlock(); } + try { + if (!completableFutures.isEmpty()) { + FutureUtil.waitForAll(completableFutures).get(AsyncOperationTimeoutSeconds, TimeUnit.SECONDS); + } + } catch (Exception e) { + log.warn("[{}] Failed wait to snapshot generate", dispatcher.getName(), e); + } finally { + bucketSnapshotExecutor.shutdown(); + } } private CompletableFuture cleanImmutableBuckets() { From 8c7e0ecac5a1998a0dbc568d969a6dea9f9a99ad Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Fri, 19 Sep 2025 20:42:53 +0800 Subject: [PATCH 07/16] test(BucketDeliveredDeliveryTrackerTest): improve snapshot persistence tests --- .../BucketDelayedDeliveryTrackerTest.java | 95 ++++++++++++------- 1 file changed, 63 insertions(+), 32 deletions(-) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerTest.java index 6ff98fa7f7004..05f8bb1505369 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerTest.java @@ -40,6 +40,7 @@ import java.util.Set; import java.util.TreeMap; import java.util.TreeSet; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -184,28 +185,34 @@ public void testContainsMessage(BucketDelayedDeliveryTracker tracker) { @Test(dataProvider = "delayedTracker", invocationCount = 10) public void testRecoverSnapshot(BucketDelayedDeliveryTracker tracker) throws Exception { - for (int i = 1; i <= 100; i++) { + final int minIndexCountPerBucket = 5; + final int messagesToSnapshot = 100; + final int triggerMessageCount = messagesToSnapshot + 1; + for (int i = 1; i <= triggerMessageCount; i++) { tracker.addMessage(i, i, i * 10); + boolean isTriggerPoint = i > minIndexCountPerBucket && (i - 1) % minIndexCountPerBucket == 0; + if (isTriggerPoint) { + final int expectedBuckets = (i - 1) / minIndexCountPerBucket; + Awaitility.await().atMost(5, TimeUnit.SECONDS) + .pollInterval(10, TimeUnit.MILLISECONDS) + .untilAsserted(() -> { + // 1. Confirm the number of immutable buckets matches the expected + assertEquals(tracker.getImmutableBuckets().asMapOfRanges().size(), expectedBuckets, + "Expected number of immutable buckets did not match."); + // 2. Confirm that the persistence Future for + // all snapshots of created immutable buckets have all completed + assertTrue(tracker.getImmutableBuckets().asMapOfRanges().values().stream() + .allMatch(bucket -> bucket.getSnapshotCreateFuture() + .map(CompletableFuture::isDone) + .orElse(true)), + "Not all snapshot creation futures were completed."); + }); + } } - assertEquals(tracker.getNumberOfDelayedMessages(), 100); - - clockTime.set(1 * 10); - - Awaitility.await().untilAsserted(() -> { - Assert.assertTrue( - tracker.getImmutableBuckets().asMapOfRanges().values().stream().noneMatch(x -> x.merging - || !x.getSnapshotCreateFuture().get().isDone())); - }); - - assertTrue(tracker.hasMessageAvailable()); - Set scheduledMessages = new TreeSet<>(); - Awaitility.await().untilAsserted(() -> { - scheduledMessages.addAll(tracker.getScheduledMessages(100)); - assertEquals(scheduledMessages.size(), 1); - }); - - tracker.addMessage(101, 101, 101 * 10); + assertEquals(tracker.getNumberOfDelayedMessages(), 101); + assertEquals(tracker.getImmutableBuckets().asMapOfRanges().size(), 20); + assertEquals(tracker.getLastMutableBucket().size(), 1); tracker.close(); @@ -430,11 +437,33 @@ public void testWithCreateFailDowngrade(BucketDelayedDeliveryTracker tracker) { @Test(dataProvider = "delayedTracker") public void testMaxIndexesPerSegment(BucketDelayedDeliveryTracker tracker) { - for (int i = 1; i <= 101; i++) { + final int minIndexCountPerBucket = 20; + final int totalMessages = 101; + final int expectedFinalBucketCount = (totalMessages - 1) / minIndexCountPerBucket; + for (int i = 1; i <= totalMessages; i++) { tracker.addMessage(i, i, i * 10); + + // The trigger point is the next message after the bucket is full. + // For example, i=21 triggers the 1st bucket, i=41 triggers the 2nd, ..., i=101 triggers the 5th + boolean isTriggerPoint = i > minIndexCountPerBucket && (i - 1) % minIndexCountPerBucket == 0; + + if (isTriggerPoint) { + final int expectedBuckets = (i - 1) / minIndexCountPerBucket; + + // Wait until the background bucket creation task is completed + Awaitility.await().atMost(5, TimeUnit.SECONDS) + .pollInterval(10, TimeUnit.MILLISECONDS) + .untilAsserted(() -> + assertEquals(tracker.getImmutableBuckets().asMapOfRanges().size(), expectedBuckets) + ); + } } - assertEquals(tracker.getImmutableBuckets().asMapOfRanges().size(), 5); + // After the loop ends, we expect to have 5 immutable buckets + assertEquals(tracker.getImmutableBuckets().asMapOfRanges().size(), expectedFinalBucketCount); + + // And, the lastMutableBucket should only have the last message (101) left. + assertEquals(tracker.getLastMutableBucket().size(), 1); tracker.getImmutableBuckets().asMapOfRanges().forEach((k, bucket) -> { assertEquals(bucket.getLastSegmentEntryId(), 4); @@ -446,21 +475,23 @@ public void testMaxIndexesPerSegment(BucketDelayedDeliveryTracker tracker) { @Test(dataProvider = "delayedTracker") public void testClear(BucketDelayedDeliveryTracker tracker) throws ExecutionException, InterruptedException, TimeoutException { - for (int i = 1; i <= 1001; i++) { + for (int i = 1; i <= 1001; i++) { tracker.addMessage(i, i, i * 10); - } + } - assertEquals(tracker.getNumberOfDelayedMessages(), 1001); - assertTrue(tracker.getImmutableBuckets().asMapOfRanges().size() > 0); - assertEquals(tracker.getLastMutableBucket().size(), 1); + assertEquals(tracker.getNumberOfDelayedMessages(), 1001); + Awaitility.await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> + Assert.assertFalse(tracker.getImmutableBuckets().asMapOfRanges().isEmpty()) + ); + assertEquals(tracker.getLastMutableBucket().size(), 1); - tracker.clear().get(1, TimeUnit.MINUTES); + tracker.clear().get(1, TimeUnit.MINUTES); - assertEquals(tracker.getNumberOfDelayedMessages(), 0); - assertEquals(tracker.getImmutableBuckets().asMapOfRanges().size(), 0); - assertEquals(tracker.getLastMutableBucket().size(), 0); - assertEquals(tracker.getSharedBucketPriorityQueue().size(), 0); + assertEquals(tracker.getNumberOfDelayedMessages(), 0); + assertEquals(tracker.getImmutableBuckets().asMapOfRanges().size(), 0); + assertEquals(tracker.getLastMutableBucket().size(), 0); + assertEquals(tracker.getSharedBucketPriorityQueue().size(), 0); - tracker.close(); + tracker.close(); } } From 9db40eb86a7483b242adc7a5f77bb8fd2c732057 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Sat, 20 Sep 2025 11:31:11 +0800 Subject: [PATCH 08/16] test(bucket): optimize BucketDelayedDeliveryTracker thread safety tests --- ...elayedDeliveryTrackerThreadSafetyTest.java | 367 +++++++++++------- 1 file changed, 236 insertions(+), 131 deletions(-) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java index 3bc96499bfdef..c9fa591f95e56 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java @@ -31,12 +31,12 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.Phaser; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; +import lombok.extern.slf4j.Slf4j; import org.apache.bookkeeper.mledger.ManagedCursor; import org.apache.pulsar.broker.service.persistent.AbstractPersistentDispatcherMultipleConsumers; import org.testng.annotations.AfterMethod; @@ -45,9 +45,10 @@ /** * Thread safety tests for BucketDelayedDeliveryTracker. - * These tests verify that the hybrid approach with StampedLock and concurrent data structures + * These tests verify that the hybrid approach with ReentrantReadWriteLock and concurrent data structures * correctly handles concurrent access patterns without deadlocks, race conditions, or data corruption. */ +@Slf4j public class BucketDelayedDeliveryTrackerThreadSafetyTest { private BucketDelayedDeliveryTracker tracker; @@ -97,88 +98,103 @@ public void setUp() throws Exception { @AfterMethod public void tearDown() throws Exception { - if (tracker != null) { - tracker.close(); - } + // First shutdown executor to stop all threads if (executorService != null) { assertTrue(MoreExecutors.shutdownAndAwaitTermination(executorService, 5, TimeUnit.SECONDS), - "Executor should shutdown cleanly"); + "Executor should shutdown cleanly"); + } + // Then close tracker safely after all threads stopped + if (tracker != null) { + tracker.close(); } } /** - * Test concurrent containsMessage() calls while adding messages. - * This tests the StampedLock optimistic read performance under contention. + * Test concurrent containsMessage() calls while adding messages sequentially. + * This tests the ReentrantReadWriteLock read performance under contention. + * addMessage is executed sequentially (as in real scenarios), while containsMessage is concurrent. */ @Test public void testConcurrentContainsMessageWithWrites() throws Exception { - final int numThreads = 16; - final int operationsPerThread = 1000; // Restore to test bucket creation properly + final int numReadThreads = 8; + final int readsPerThread = 1000; + final int totalMessages = 5000; final CountDownLatch startLatch = new CountDownLatch(1); - final CountDownLatch doneLatch = new CountDownLatch(numThreads); + final CountDownLatch readersDone = new CountDownLatch(numReadThreads); final AtomicInteger errors = new AtomicInteger(0); final AtomicReference firstException = new AtomicReference<>(); + final AtomicInteger messagesAdded = new AtomicInteger(0); - // Start reader threads - for (int i = 0; i < numThreads / 2; i++) { - final int threadId = i; + // Start reader threads - these will run concurrently + for (int i = 0; i < numReadThreads; i++) { executorService.submit(() -> { try { startLatch.await(); - for (int j = 0; j < operationsPerThread; j++) { - long ledgerId = threadId * 1000 + j; - long entryId = j; + // Continuously read for a period while messages are being added + long endTime = System.currentTimeMillis() + 10000; + int readCount = 0; + while (System.currentTimeMillis() < endTime && readCount < readsPerThread) { + // Check for messages across the range that might be added + long ledgerId = 1000 + (readCount % totalMessages); + long entryId = readCount % 100; // This should not throw exceptions or block indefinitely tracker.containsMessage(ledgerId, entryId); + readCount++; + if (readCount % 100 == 0) { + Thread.sleep(1); + } } } catch (Exception e) { errors.incrementAndGet(); firstException.compareAndSet(null, e); e.printStackTrace(); } finally { - doneLatch.countDown(); + readersDone.countDown(); } }); } - // Start writer threads - for (int i = numThreads / 2; i < numThreads; i++) { - final int threadId = i; - executorService.submit(() -> { - try { - startLatch.await(); - for (int j = 0; j < operationsPerThread; j++) { - long ledgerId = threadId * 1000 + j; - long entryId = j; - long deliverAt = System.currentTimeMillis() + 10000; // 10s delay - tracker.addMessage(ledgerId, entryId, deliverAt); + // Start the single writer thread - sequential addMessage calls + executorService.submit(() -> { + try { + startLatch.await(); + for (int i = 0; i < totalMessages; i++) { + long ledgerId = 1000 + i; + long entryId = i % 100; + long deliverAt = System.currentTimeMillis() + 10000; + boolean added = tracker.addMessage(ledgerId, entryId, deliverAt); + if (added) { + messagesAdded.incrementAndGet(); + } + // Small delay to simulate real processing time + if (i % 100 == 0) { + Thread.sleep(1); } - } catch (Exception e) { - errors.incrementAndGet(); - firstException.compareAndSet(null, e); - e.printStackTrace(); - } finally { - doneLatch.countDown(); } - }); - } + } catch (Exception e) { + errors.incrementAndGet(); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } + }); startLatch.countDown(); - assertTrue(doneLatch.await(30, TimeUnit.SECONDS), "Test should complete within 30 seconds"); + assertTrue(readersDone.await(30, TimeUnit.SECONDS), "Readers should complete within 30 seconds"); if (errors.get() > 0) { Exception exception = firstException.get(); if (exception != null) { - System.err.println("First exception caught: " + exception.getMessage()); + log.error("First exception caught: " + exception.getMessage()); exception.printStackTrace(); } } assertEquals(errors.get(), 0, "No exceptions should occur during concurrent operations"); + assertTrue(messagesAdded.get() > 0, "Some messages should have been added"); } /** * Test concurrent nextDeliveryTime() calls. - * This verifies the StampedLock implementation for read-heavy operations. + * This verifies the ReentrantReadWriteLock implementation for read-heavy operations. */ @Test public void testConcurrentNextDeliveryTime() throws Exception { @@ -224,75 +240,149 @@ public void testConcurrentNextDeliveryTime() throws Exception { */ @Test public void testDeadlockDetection() throws Exception { - final int numThreads = 32; - final int operationsPerThread = 100; - // Use Phaser for better concurrency coordination - final Phaser startPhaser = new Phaser(numThreads + 1); // +1 for main thread - final Phaser endPhaser = new Phaser(numThreads + 1); // +1 for main thread + final int numReadThreads = 30; + final int operationsPerThread = 200; + final int writeOperations = 1000; + + final CountDownLatch startLatch = new CountDownLatch(1); + final CountDownLatch writerDone = new CountDownLatch(1); + final CountDownLatch readersDone = new CountDownLatch(numReadThreads); final AtomicBoolean deadlockDetected = new AtomicBoolean(false); final AtomicInteger completedOperations = new AtomicInteger(0); - - // Mixed workload: reads, writes, and metric queries - for (int i = 0; i < numThreads; i++) { + final AtomicInteger writesCompleted = new AtomicInteger(0); + final AtomicReference firstException = new AtomicReference<>(); + // Single writer thread - executes addMessage sequentially + executorService.submit(() -> { + try { + startLatch.await(); + for (int i = 0; i < writeOperations; i++) { + try { + long ledgerId = 50000 + i; + long entryId = i; + tracker.addMessage(ledgerId, entryId, System.currentTimeMillis() + 10000); + writesCompleted.incrementAndGet(); + completedOperations.incrementAndGet(); + + // Small delay to allow read threads to interleave + if (i % 50 == 0) { + Thread.sleep(1); + } + } catch (Exception e) { + if (!(e instanceof IllegalArgumentException)) { + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); + break; + } + completedOperations.incrementAndGet(); + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + deadlockDetected.set(true); + } catch (Exception e) { + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } finally { + writerDone.countDown(); + } + }); + // Multiple reader threads - execute read operations concurrently + for (int i = 0; i < numReadThreads; i++) { final int threadId = i; - final int workloadType = i % 4; - + final int readOperationType = i % 3; executorService.submit(() -> { try { - // Wait for all threads to be ready - startPhaser.arriveAndAwaitAdvance(); + startLatch.await(); - for (int j = 0; j < operationsPerThread; j++) { + // Continue reading until writer is done, plus some extra operations + int operationCount = 0; + while ((writerDone.getCount() > 0 || operationCount < operationsPerThread)) { try { - switch (workloadType) { - case 0: // containsMessage calls - tracker.containsMessage(threadId * 1000 + j, j); - break; - case 1: // addMessage calls - tracker.addMessage(threadId * 1000 + j, j, System.currentTimeMillis() + 5000); + switch (readOperationType) { + case 0: + // Check both existing and potentially non-existing messages + long ledgerId = 50000 + (operationCount % (writeOperations + 100)); + long entryId = operationCount % 1000; + tracker.containsMessage(ledgerId, entryId); break; - case 2: // nextDeliveryTime calls + case 1: tracker.nextDeliveryTime(); break; - case 3: // getNumberOfDelayedMessages calls + case 2: tracker.getNumberOfDelayedMessages(); break; } completedOperations.incrementAndGet(); + operationCount++; + + // Small delay to prevent excessive CPU usage + if (operationCount % 100 == 0) { + Thread.sleep(1); + } } catch (IllegalArgumentException e) { - // IllegalArgumentException is expected for some operations - // (e.g., calling nextDeliveryTime on empty queue, invalid ledger IDs) - // This is not a deadlock, just normal validation + // Expected for some operations (e.g., nextDeliveryTime on empty queue) completedOperations.incrementAndGet(); + operationCount++; + } catch (Exception e) { + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); + break; + } + + // Break if we've done enough operations and writer is done + if (writerDone.getCount() == 0 && operationCount >= operationsPerThread) { + break; } } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + deadlockDetected.set(true); } catch (Exception e) { - // Only unexpected exceptions indicate potential deadlocks - if (!(e instanceof IllegalArgumentException)) { - deadlockDetected.set(true); - e.printStackTrace(); - } + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); } finally { - // Signal completion - endPhaser.arriveAndDeregister(); + readersDone.countDown(); } }); } + // Start all threads + startLatch.countDown(); + // Wait for completion with timeout to detect deadlocks + boolean writerCompleted = false; + boolean readersCompleted = false; - // Start all threads at once - startPhaser.arriveAndAwaitAdvance(); - - // Wait for all threads to complete with timeout to detect potential deadlocks try { - endPhaser.awaitAdvanceInterruptibly(endPhaser.arrive(), 60, TimeUnit.SECONDS); - } catch (Exception e) { - // Timeout or interrupt indicates potential deadlock + writerCompleted = writerDone.await(30, TimeUnit.SECONDS); + readersCompleted = readersDone.await(30, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); deadlockDetected.set(true); - e.printStackTrace(); } - - assertTrue(!deadlockDetected.get(), "No deadlocks should be detected"); + // Check for deadlock indicators + if (!writerCompleted || !readersCompleted) { + deadlockDetected.set(true); + log.error("Test timed out - potential deadlock detected. Writer completed: {}, Readers completed: {}", + writerCompleted, readersCompleted); + } + // Assert results + if (deadlockDetected.get()) { + Exception e = firstException.get(); + if (e != null) { + throw new AssertionError("Deadlock or exception detected during test execution", e); + } else { + throw new AssertionError("Deadlock detected - test did not complete within timeout"); + } + } + // Verify that operations actually completed assertTrue(completedOperations.get() > 0, "Some operations should complete"); + assertTrue(writesCompleted.get() > 0, "Some write operations should complete"); + + log.info("Deadlock test completed successfully. Total operations: {}, Writes completed: {}", + completedOperations.get(), writesCompleted.get()); } /** @@ -301,85 +391,100 @@ public void testDeadlockDetection() throws Exception { */ @Test public void testDataConsistencyUnderConcurrency() throws Exception { - final int numWriteThreads = 8; final int numReadThreads = 16; - final int messagesPerWriter = 500; + final int totalMessages = 4000; + final int readsPerThread = 1000; final CountDownLatch startLatch = new CountDownLatch(1); - final CountDownLatch writersDone = new CountDownLatch(numWriteThreads); final CountDownLatch readersDone = new CountDownLatch(numReadThreads); + final AtomicInteger errors = new AtomicInteger(0); + final AtomicReference firstException = new AtomicReference<>(); final AtomicInteger foundMessages = new AtomicInteger(0); final AtomicInteger totalMessagesAdded = new AtomicInteger(0); - - // Writer threads add messages - for (int i = 0; i < numWriteThreads; i++) { - final int writerId = i; - executorService.submit(() -> { - try { - startLatch.await(); - for (int j = 0; j < messagesPerWriter; j++) { - long ledgerId = writerId * 10000 + j; - long entryId = j; - boolean added = tracker.addMessage(ledgerId, entryId, System.currentTimeMillis() + 30000); - if (added) { - totalMessagesAdded.incrementAndGet(); - } - } - } catch (Exception e) { - // Ignore exceptions for this test - } finally { - writersDone.countDown(); - } - }); - } - - // Reader threads check for messages + // Start reader threads - these will run concurrently for (int i = 0; i < numReadThreads; i++) { final int readerId = i; executorService.submit(() -> { try { startLatch.await(); + // Continuously read for a period while messages are being added + long endTime = System.currentTimeMillis() + 12000; + int readCount = 0; + while (System.currentTimeMillis() < endTime && readCount < readsPerThread) { + // Check for messages across the range that might be added + int messageIndex = readCount % totalMessages; + long ledgerId = 10000 + messageIndex; + long entryId = messageIndex % 100; + + if (tracker.containsMessage(ledgerId, entryId)) { + foundMessages.incrementAndGet(); + } + readCount++; - // Read for a while to catch messages being added - long endTime = System.currentTimeMillis() + 5000; // Read for 5 seconds - while (System.currentTimeMillis() < endTime) { - for (int writerId = 0; writerId < numWriteThreads; writerId++) { - for (int j = 0; j < messagesPerWriter; j++) { - long ledgerId = writerId * 10000 + j; - long entryId = j; - if (tracker.containsMessage(ledgerId, entryId)) { - foundMessages.incrementAndGet(); - } - } + if (readCount % 200 == 0) { + Thread.sleep(1); } - Thread.sleep(10); // Small delay to allow writes } } catch (Exception e) { - // Ignore exceptions for this test + errors.incrementAndGet(); + firstException.compareAndSet(null, e); + e.printStackTrace(); } finally { readersDone.countDown(); } }); } - + // Start the single writer thread - sequential addMessage calls + executorService.submit(() -> { + try { + startLatch.await(); + for (int i = 0; i < totalMessages; i++) { + long ledgerId = 10000 + i; + long entryId = i % 100; + long deliverAt = System.currentTimeMillis() + 30000; + boolean added = tracker.addMessage(ledgerId, entryId, deliverAt); + if (added) { + totalMessagesAdded.incrementAndGet(); + } + // Small delay to simulate real processing time and allow reads + if (i % 200 == 0) { + Thread.sleep(2); + } + } + } catch (Exception e) { + errors.incrementAndGet(); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } + }); startLatch.countDown(); - - assertTrue(writersDone.await(30, TimeUnit.SECONDS), "Writers should complete"); - assertTrue(readersDone.await(30, TimeUnit.SECONDS), "Readers should complete"); - + assertTrue(readersDone.await(40, TimeUnit.SECONDS), "Readers should complete within 40 seconds"); + // Check for errors during concurrent operations + if (errors.get() > 0) { + Exception exception = firstException.get(); + if (exception != null) { + log.error("First exception caught: " + exception.getMessage()); + exception.printStackTrace(); + } + } + assertEquals(errors.get(), 0, "No exceptions should occur during concurrent operations"); // Verify final consistency long finalMessageCount = tracker.getNumberOfDelayedMessages(); assertTrue(finalMessageCount >= 0, "Message count should be non-negative"); - - // The exact counts may vary due to timing, but we should have some successful operations + // The exact counts may vary due to timing, but we should have successful operations assertTrue(totalMessagesAdded.get() > 0, "Some messages should have been added"); + assertTrue(foundMessages.get() >= 0, "Found messages count should be non-negative"); + + // Log results for analysis + log.info("Total messages added: {}, Found messages: {}, Final message count: {}", + totalMessagesAdded.get(), foundMessages.get(), finalMessageCount); } /** - * Test optimistic read performance under varying contention levels. - * This helps validate that the StampedLock optimistic reads are working efficiently. + * Test read performance under varying contention levels. + * This helps validate that the ReentrantReadWriteLock reads are working efficiently. */ @Test - public void testOptimisticReadPerformance() throws Exception { + public void testReadPerformanceUnderContention() throws Exception { // Add baseline messages for (int i = 0; i < 1000; i++) { tracker.addMessage(i, i, System.currentTimeMillis() + 60000); From 9f09e8698480bf8838ed1cc652ae3f1fdfe6dda7 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Sat, 20 Sep 2025 12:45:00 +0800 Subject: [PATCH 09/16] test(BucketDelayedDeliveryTracker): extend thread safety tests with new scenarios --- ...elayedDeliveryTrackerThreadSafetyTest.java | 484 ++++++++++++++++++ 1 file changed, 484 insertions(+) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java index c9fa591f95e56..31362b68436dd 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java @@ -27,6 +27,7 @@ import com.google.common.util.concurrent.MoreExecutors; import io.netty.util.Timer; import java.time.Clock; +import java.util.NavigableSet; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; @@ -38,6 +39,7 @@ import java.util.concurrent.atomic.AtomicReference; import lombok.extern.slf4j.Slf4j; import org.apache.bookkeeper.mledger.ManagedCursor; +import org.apache.bookkeeper.mledger.Position; import org.apache.pulsar.broker.service.persistent.AbstractPersistentDispatcherMultipleConsumers; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; @@ -535,4 +537,486 @@ public void testReadPerformanceUnderContention() throws Exception { assertTrue(throughput > 10000, "Should achieve at least 10K ops/sec with " + numThreads + " threads"); } } + + /** + * Test concurrent getScheduledMessages() calls with read operations. + * getScheduledMessages() uses write lock while read operations use read lock. + * Messages are added beforehand to avoid concurrent addMessage calls. + */ + @Test + public void testConcurrentGetScheduledMessagesWithReads() throws Exception { + // Add messages that will be ready for delivery after a short delay + final long baseTime = System.currentTimeMillis(); + final int totalMessages = 500; + + // Add messages with delivery time slightly in the future, then wait for them to become ready + for (int i = 0; i < totalMessages; i++) { + tracker.addMessage(i, i, baseTime + 1000); + } + assertEquals(tracker.getNumberOfDelayedMessages(), totalMessages, "All messages should be added"); + // Wait for messages to become ready for delivery + Thread.sleep(3000); + final int numReadThreads = 12; + final int numScheduleThreads = 4; + final CountDownLatch startLatch = new CountDownLatch(1); + final CountDownLatch readersDone = new CountDownLatch(numReadThreads); + final CountDownLatch schedulersDone = new CountDownLatch(numScheduleThreads); + final AtomicInteger errors = new AtomicInteger(0); + final AtomicReference firstException = new AtomicReference<>(); + final AtomicInteger totalMessagesRetrieved = new AtomicInteger(0); + // Start read threads (containsMessage and nextDeliveryTime) + for (int i = 0; i < numReadThreads; i++) { + final int threadId = i; + executorService.submit(() -> { + try { + startLatch.await(); + for (int j = 0; j < 1000; j++) { + if (threadId % 2 == 0) { + tracker.containsMessage(j % totalMessages, j % totalMessages); + } else { + try { + tracker.nextDeliveryTime(); + } catch (IllegalArgumentException e) { + // Expected when no messages available + } + } + if (j % 100 == 0) { + Thread.sleep(1); + } + } + } catch (Exception e) { + errors.incrementAndGet(); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } finally { + readersDone.countDown(); + } + }); + } + // Start getScheduledMessages threads - continue until all messages are retrieved + for (int i = 0; i < numScheduleThreads; i++) { + executorService.submit(() -> { + try { + startLatch.await(); + int consecutiveEmptyReturns = 0; + final int maxConsecutiveEmpty = 5; + + while (totalMessagesRetrieved.get() < totalMessages + && consecutiveEmptyReturns < maxConsecutiveEmpty) { + NavigableSet messages = tracker.getScheduledMessages(50); + int retrieved = messages.size(); + totalMessagesRetrieved.addAndGet(retrieved); + + if (retrieved == 0) { + consecutiveEmptyReturns++; + Thread.sleep(10); + } else { + consecutiveEmptyReturns = 0; + Thread.sleep(5); + } + } + } catch (Exception e) { + errors.incrementAndGet(); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } finally { + schedulersDone.countDown(); + } + }); + } + startLatch.countDown(); + assertTrue(readersDone.await(30, TimeUnit.SECONDS), "Readers should complete within 30 seconds"); + assertTrue(schedulersDone.await(30, TimeUnit.SECONDS), "Schedulers should complete within 30 seconds"); + if (errors.get() > 0) { + Exception exception = firstException.get(); + if (exception != null) { + throw new AssertionError("Concurrent getScheduledMessages test failed", exception); + } + } + assertEquals(errors.get(), 0, "No exceptions should occur during concurrent operations"); + + // Verify that most or all messages were retrieved + assertEquals(totalMessagesRetrieved.get(), 500, "All messages should be retrieved"); + + log.info("Total messages retrieved: {} out of {}", totalMessagesRetrieved.get(), totalMessages); + } + + /** + * Test concurrent clear() operations with read operations. + * This verifies that clear() properly coordinates with ongoing read operations. + * Messages are added beforehand, then clear() is tested with concurrent reads. + */ + @Test + public void testConcurrentClearWithReads() throws Exception { + final int initialMessages = 1000; + final int numReadThreads = 10; + final CountDownLatch startLatch = new CountDownLatch(1); + final CountDownLatch readersDone = new CountDownLatch(numReadThreads); + final AtomicInteger errors = new AtomicInteger(0); + final AtomicReference firstException = new AtomicReference<>(); + final AtomicBoolean clearCompleted = new AtomicBoolean(false); + // Add initial messages (single thread) + for (int i = 0; i < initialMessages; i++) { + tracker.addMessage(i, i, System.currentTimeMillis() + 60000); + } + // Start read threads that will run during clear operation + for (int i = 0; i < numReadThreads; i++) { + final int threadId = i; + executorService.submit(() -> { + try { + startLatch.await(); + while (!clearCompleted.get()) { + switch (threadId % 3) { + case 0: + tracker.containsMessage(threadId, threadId); + break; + case 1: + try { + tracker.nextDeliveryTime(); + } catch (IllegalArgumentException e) { + // Expected when no messages available + } + break; + case 2: + tracker.getNumberOfDelayedMessages(); + break; + } + Thread.sleep(1); + } + // Continue reading for a bit after clear + for (int j = 0; j < 100; j++) { + tracker.containsMessage(j, j); + Thread.sleep(1); + } + } catch (Exception e) { + errors.incrementAndGet(); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } finally { + readersDone.countDown(); + } + }); + } + // Start clear operation after a short delay + executorService.submit(() -> { + try { + startLatch.await(); + Thread.sleep(100); + tracker.clear().get(30, TimeUnit.SECONDS); + clearCompleted.set(true); + } catch (Exception e) { + errors.incrementAndGet(); + firstException.compareAndSet(null, e); + e.printStackTrace(); + clearCompleted.set(true); + } + }); + startLatch.countDown(); + assertTrue(readersDone.await(60, TimeUnit.SECONDS), "Readers should complete within 60 seconds"); + if (errors.get() > 0) { + Exception exception = firstException.get(); + if (exception != null) { + throw new AssertionError("Concurrent clear test failed", exception); + } + } + assertEquals(errors.get(), 0, "No exceptions should occur during concurrent clear operations"); + assertEquals(tracker.getNumberOfDelayedMessages(), 0, "All messages should be cleared"); + } + + /** + * Test concurrent close() operations. + * This verifies that close() properly handles concurrent access and shuts down cleanly. + * Messages are added beforehand to test close() behavior with existing data. + */ + @Test + public void testConcurrentClose() throws Exception { + final int numReadThreads = 8; + final CountDownLatch startLatch = new CountDownLatch(1); + final CountDownLatch readersDone = new CountDownLatch(numReadThreads); + final AtomicInteger errors = new AtomicInteger(0); + final AtomicReference firstException = new AtomicReference<>(); + final AtomicBoolean closeInitiated = new AtomicBoolean(false); + // Add some messages first (single thread) + for (int i = 0; i < 100; i++) { + tracker.addMessage(i, i, System.currentTimeMillis() + 60000); + } + // Start read threads that will be interrupted by close + for (int i = 0; i < numReadThreads; i++) { + final int threadId = i; + executorService.submit(() -> { + try { + startLatch.await(); + while (!closeInitiated.get()) { + try { + switch (threadId % 4) { + case 0: + tracker.containsMessage(threadId, threadId); + break; + case 1: + tracker.nextDeliveryTime(); + break; + case 2: + tracker.getNumberOfDelayedMessages(); + break; + case 3: + tracker.getScheduledMessages(10); + break; + } + } catch (IllegalArgumentException e) { + // Expected for some operations when tracker is being closed + } + Thread.sleep(1); + } + } catch (Exception e) { + // Some exceptions may be expected during close + if (!closeInitiated.get()) { + errors.incrementAndGet(); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } + } finally { + readersDone.countDown(); + } + }); + } + // Start close operation after a short delay + executorService.submit(() -> { + try { + startLatch.await(); + Thread.sleep(100); + closeInitiated.set(true); + tracker.close(); + } catch (Exception e) { + errors.incrementAndGet(); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } + }); + startLatch.countDown(); + assertTrue(readersDone.await(30, TimeUnit.SECONDS), "Readers should complete within 30 seconds"); + if (errors.get() > 0) { + Exception exception = firstException.get(); + if (exception != null) { + log.warn("Exception during concurrent close test (may be expected): " + exception.getMessage()); + } + } + // Create a new tracker for the next test since this one is closed + tracker = new BucketDelayedDeliveryTracker( + dispatcher, timer, 1000, Clock.systemUTC(), true, storage, + 100, 1000, 100, 10 + ); + } + + /** + * Test mixed read operations with sequential addMessage and concurrent getScheduledMessages. + * This tests the ReentrantReadWriteLock behavior when read and write operations are mixed. + * addMessage is executed in single thread, while reads and getScheduledMessages are concurrent. + * Ensures all deliverable messages are retrieved before test completion. + */ + @Test + public void testMixedReadWriteOperationsDeadlockDetection() throws Exception { + final int numReadThreads = 16; + final int numScheduleThreads = 4; + final int totalMessages = 2000; + final int readsPerThread = 500; + + final CountDownLatch startLatch = new CountDownLatch(1); + final CountDownLatch readersDone = new CountDownLatch(numReadThreads); + final CountDownLatch schedulersDone = new CountDownLatch(numScheduleThreads); + final CountDownLatch writerDone = new CountDownLatch(1); + final AtomicBoolean deadlockDetected = new AtomicBoolean(false); + final AtomicInteger completedOperations = new AtomicInteger(0); + final AtomicReference firstException = new AtomicReference<>(); + final AtomicInteger messagesAdded = new AtomicInteger(0); + final AtomicInteger deliverableMessagesCount = new AtomicInteger(0); + final AtomicInteger totalMessagesRetrieved = new AtomicInteger(0); + // Single writer thread for addMessage (sequential execution) + executorService.submit(() -> { + try { + startLatch.await(); + final long baseTime = System.currentTimeMillis(); + + for (int i = 0; i < totalMessages; i++) { + try { + long ledgerId = 10000 + i; + long entryId = i % 1000; + + // Create mix of messages: some ready for delivery, some delayed + long deliverAt; + if (i % 3 == 0) { + // Messages that will be ready for delivery after a short delay + deliverAt = baseTime + 500; + deliverableMessagesCount.incrementAndGet(); + } else { + // Messages for future delivery (much later) + deliverAt = baseTime + 30000; + } + + boolean added = tracker.addMessage(ledgerId, entryId, deliverAt); + if (added) { + messagesAdded.incrementAndGet(); + } + completedOperations.incrementAndGet(); + + if (i % 200 == 0) { + Thread.sleep(1); + } + } catch (Exception e) { + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); + break; + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + deadlockDetected.set(true); + } catch (Exception e) { + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } finally { + writerDone.countDown(); + } + }); + // Start read threads (using read locks) + for (int i = 0; i < numReadThreads; i++) { + final int threadId = i; + executorService.submit(() -> { + try { + startLatch.await(); + // Continue reading until writer is done, plus some extra operations + int operationCount = 0; + while ((writerDone.getCount() > 0 || operationCount < readsPerThread)) { + try { + switch (threadId % 3) { + case 0: + long ledgerId = 10000 + (operationCount % (totalMessages + 100)); + long entryId = operationCount % 1000; + tracker.containsMessage(ledgerId, entryId); + break; + case 1: + tracker.nextDeliveryTime(); + break; + case 2: + tracker.getNumberOfDelayedMessages(); + break; + } + completedOperations.incrementAndGet(); + operationCount++; + if (operationCount % 100 == 0) { + Thread.sleep(1); + } + } catch (IllegalArgumentException e) { + // Expected for some operations + completedOperations.incrementAndGet(); + operationCount++; + } catch (Exception e) { + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); + break; + } + if (writerDone.getCount() == 0 && operationCount >= readsPerThread) { + break; + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + deadlockDetected.set(true); + } catch (Exception e) { + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } finally { + readersDone.countDown(); + } + }); + } + // Start getScheduledMessages threads (using write locks) + for (int i = 0; i < numScheduleThreads; i++) { + executorService.submit(() -> { + try { + startLatch.await(); + + // Wait for writer to finish and messages to become deliverable + writerDone.await(); + Thread.sleep(1000); // Wait 1 second for messages to become ready for delivery + + int consecutiveEmptyReturns = 0; + final int maxConsecutiveEmpty = 5; + + // Continue until we've retrieved all deliverable messages or hit max empty returns + while (totalMessagesRetrieved.get() < deliverableMessagesCount.get() + && consecutiveEmptyReturns < maxConsecutiveEmpty) { + try { + NavigableSet messages = tracker.getScheduledMessages(50); + int retrieved = messages.size(); + totalMessagesRetrieved.addAndGet(retrieved); + completedOperations.incrementAndGet(); + + if (retrieved == 0) { + consecutiveEmptyReturns++; + Thread.sleep(5); // Short wait for more messages + } else { + consecutiveEmptyReturns = 0; + Thread.sleep(2); // Short processing delay + } + + } catch (Exception e) { + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); + break; + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + deadlockDetected.set(true); + } catch (Exception e) { + deadlockDetected.set(true); + firstException.compareAndSet(null, e); + e.printStackTrace(); + } finally { + schedulersDone.countDown(); + } + }); + } + // Start all threads + startLatch.countDown(); + + // Wait for completion with reasonable timeout to detect deadlocks + boolean writerCompleted = writerDone.await(10, TimeUnit.SECONDS); + boolean readersCompleted = readersDone.await(15, TimeUnit.SECONDS); + boolean schedulersCompleted = schedulersDone.await(20, TimeUnit.SECONDS); + if (!writerCompleted || !readersCompleted || !schedulersCompleted) { + deadlockDetected.set(true); + log.error("Test timed out - potential deadlock detected. Writer: {}, Readers: {}, Schedulers: {}", + writerCompleted, readersCompleted, schedulersCompleted); + } + if (deadlockDetected.get()) { + Exception e = firstException.get(); + if (e != null) { + throw new AssertionError("Deadlock or exception detected during mixed operations test", e); + } else { + throw new AssertionError("Deadlock detected - test did not complete within timeout"); + } + } + + // Awaitility.await().atMost(30, TimeUnit.SECONDS).untilAsserted(() -> + // assertEquals(deliverableMessagesCount.get(), totalMessages)); + + // Verify operations completed successfully + assertTrue(completedOperations.get() > 0, "Some operations should complete"); + assertTrue(messagesAdded.get() > 0, "Some messages should have been added"); + assertTrue(deliverableMessagesCount.get() > 0, "Some messages should be deliverable"); + + // Verify that all deliverable messages were retrieved + assertEquals(totalMessagesRetrieved.get(), deliverableMessagesCount.get(), + "All deliverable messages should be retrieved"); + log.info("Mixed operations test completed successfully. Total operations: {}, Messages added: {}, " + + "Deliverable messages: {}, Retrieved messages: {}", + completedOperations.get(), messagesAdded.get(), + deliverableMessagesCount.get(), totalMessagesRetrieved.get()); + } } \ No newline at end of file From c6dd2862b34c2a2d9e4b8e0ae4e39fa1447117f1 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Sat, 20 Sep 2025 20:59:39 +0800 Subject: [PATCH 10/16] feat(benchmark): add JMH benchmarks for BucketDelayedDeliveryTracker --- .../broker/MockPersistentDispatcher.java | 860 ++++++++++++++++++ ...BucketDelayedDeliveryTrackerBenchmark.java | 254 ++++++ ...DelayedDeliveryTrackerSimpleBenchmark.java | 408 --------- .../bucket/MockBucketSnapshotStorage.java | 77 ++ .../apache/pulsar/broker/package-info.java | 27 + 5 files changed, 1218 insertions(+), 408 deletions(-) create mode 100644 microbench/src/main/java/org/apache/pulsar/broker/MockPersistentDispatcher.java create mode 100644 microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java delete mode 100644 microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerSimpleBenchmark.java create mode 100644 microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/MockBucketSnapshotStorage.java create mode 100644 microbench/src/main/java/org/apache/pulsar/broker/package-info.java diff --git a/microbench/src/main/java/org/apache/pulsar/broker/MockPersistentDispatcher.java b/microbench/src/main/java/org/apache/pulsar/broker/MockPersistentDispatcher.java new file mode 100644 index 0000000000000..198091c1a42be --- /dev/null +++ b/microbench/src/main/java/org/apache/pulsar/broker/MockPersistentDispatcher.java @@ -0,0 +1,860 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.pulsar.broker; + +import io.netty.buffer.ByteBuf; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import lombok.extern.slf4j.Slf4j; +import org.apache.bookkeeper.mledger.Entry; +import org.apache.bookkeeper.mledger.ManagedCursor; +import org.apache.bookkeeper.mledger.ManagedLedgerException; +import org.apache.bookkeeper.mledger.Position; +import org.apache.bookkeeper.mledger.PositionFactory; +import org.apache.bookkeeper.mledger.impl.ActiveManagedCursorContainerImpl; +import org.apache.bookkeeper.mledger.impl.MockManagedCursor; +import org.apache.pulsar.broker.intercept.BrokerInterceptor; +import org.apache.pulsar.broker.loadbalance.extensions.data.BrokerLookupData; +import org.apache.pulsar.broker.service.AnalyzeBacklogResult; +import org.apache.pulsar.broker.service.BrokerService; +import org.apache.pulsar.broker.service.BrokerServiceException; +import org.apache.pulsar.broker.service.Consumer; +import org.apache.pulsar.broker.service.Dispatcher; +import org.apache.pulsar.broker.service.GetStatsOptions; +import org.apache.pulsar.broker.service.Producer; +import org.apache.pulsar.broker.service.RedeliveryTracker; +import org.apache.pulsar.broker.service.Replicator; +import org.apache.pulsar.broker.service.Subscription; +import org.apache.pulsar.broker.service.SubscriptionOption; +import org.apache.pulsar.broker.service.Topic; +import org.apache.pulsar.broker.service.TopicAttributes; +import org.apache.pulsar.broker.service.TransportCnx; +import org.apache.pulsar.broker.service.persistent.AbstractPersistentDispatcherMultipleConsumers; +import org.apache.pulsar.broker.service.plugin.EntryFilter; +import org.apache.pulsar.broker.stats.ClusterReplicationMetrics; +import org.apache.pulsar.broker.stats.NamespaceStats; +import org.apache.pulsar.client.api.MessageId; +import org.apache.pulsar.client.api.transaction.TxnID; +import org.apache.pulsar.common.api.proto.CommandAck; +import org.apache.pulsar.common.api.proto.CommandSubscribe; +import org.apache.pulsar.common.api.proto.KeySharedMeta; +import org.apache.pulsar.common.policies.data.BacklogQuota; +import org.apache.pulsar.common.policies.data.EntryFilters; +import org.apache.pulsar.common.policies.data.HierarchyTopicPolicies; +import org.apache.pulsar.common.policies.data.PersistentTopicInternalStats; +import org.apache.pulsar.common.policies.data.Policies; +import org.apache.pulsar.common.policies.data.stats.TopicMetricBean; +import org.apache.pulsar.common.policies.data.stats.TopicStatsImpl; +import org.apache.pulsar.common.protocol.schema.SchemaData; +import org.apache.pulsar.common.protocol.schema.SchemaVersion; +import org.apache.pulsar.common.util.Codec; +import org.apache.pulsar.policies.data.loadbalancer.NamespaceBundleStats; +import org.apache.pulsar.utils.StatsOutputStream; + +@Slf4j +public class MockPersistentDispatcher extends AbstractPersistentDispatcherMultipleConsumers { + + private final MockManagedCursor cursor; + private final MockSubscription subscription; + private final MockTopic topic; + + private MockPersistentDispatcher(MockTopic topic, MockSubscription subscription, + MockManagedCursor cursor) { + super(subscription, new ServiceConfiguration()); + this.topic = topic; + this.subscription = subscription; + this.cursor = cursor; + } + + public static MockPersistentDispatcher create() throws Exception { + // LocalBookkeeperEnsemble bkEnsemble = new LocalBookkeeperEnsemble(3, 0, () -> 0); + // bkEnsemble.start(); + // + // // Start broker + // ServiceConfiguration config = new ServiceConfiguration(); + // config.setClusterName("use"); + // config.setWebServicePort(Optional.of(0)); + // config.setMetadataStoreUrl("zk:127.0.0.1:" + bkEnsemble.getZookeeperPort()); + // config.setBrokerShutdownTimeoutMs(0L); + // config.setLoadBalancerOverrideBrokerNicSpeedGbps(Optional.of(1.0d)); + // config.setBrokerServicePort(Optional.of(0)); + // config.setLoadManagerClassName(SimpleLoadManagerImpl.class.getName()); + // config.setBrokerServicePortTls(Optional.of(0)); + // config.setWebServicePortTls(Optional.of(0)); + // config.setAdvertisedAddress("localhost"); + // + // @Cleanup + // PulsarService pulsarService = new PulsarService(config); + // pulsarService.start(); + // + // ActiveManagedCursorContainerImpl container = new ActiveManagedCursorContainerImpl(); + // MockManagedCursor cursor = MockManagedCursor.createCursor(container, "cursor", + // PositionFactory.create(0, 1)); + // + // + // + // TopicName topicName = TopicName.get("persistent://pulsar/default/ns/topic"); + // PersistentTopic topic = new PersistentTopic(topicName.toString(), cursor.getManagedLedger(), + // pulsarService.getBrokerService()); + // + // PersistentSubscription subscription = new PersistentSubscription(topic, "sub", cursor, false); + // + // return new MockPersistentDispatcher(topic, subscription, cursor); + + + ActiveManagedCursorContainerImpl container = new ActiveManagedCursorContainerImpl(); + MockManagedCursor cursor = MockManagedCursor.createCursor(container, "test-cursor", + PositionFactory.create(0, 0)); + + MockTopic topic = new MockTopic(); + MockSubscription subscription = new MockSubscription(topic); + + return new MockPersistentDispatcher(topic, subscription, cursor); + } + + @Override + public void unBlockDispatcherOnUnackedMsgs() { + + } + + @Override + public void readMoreEntriesAsync() { + + } + + @Override + protected boolean isConsumersExceededOnSubscription() { + return false; + } + + @Override + protected void reScheduleRead() { + + } + + @Override + public String getName() { + return topic.getName() + " / " + Codec.decode(cursor.getName()); + } + + @Override + public boolean isBlockedDispatcherOnUnackedMsgs() { + return false; + } + + @Override + public int getTotalUnackedMessages() { + return 0; + } + + @Override + public void blockDispatcherOnUnackedMsgs() { + + } + + @Override + public long getNumberOfMessagesInReplay() { + return 0; + } + + @Override + public boolean isHavePendingRead() { + return false; + } + + @Override + public boolean isHavePendingReplayRead() { + return false; + } + + @Override + public ManagedCursor getCursor() { + return cursor; + } + + @Override + public Topic getTopic() { + return topic; + } + + @Override + public Subscription getSubscription() { + return subscription; + } + + @Override + public long getDelayedTrackerMemoryUsage() { + return 0; + } + + @Override + public Map getBucketDelayedIndexStats() { + return Map.of(); + } + + @Override + public boolean isClassic() { + return false; + } + + @Override + public void readEntriesComplete(List entries, Object ctx) { + + } + + @Override + public void readEntriesFailed(ManagedLedgerException exception, Object ctx) { + + } + + @Override + public boolean isConsumerAvailable(Consumer consumer) { + return false; + } + + @Override + public CompletableFuture addConsumer(Consumer consumer) { + return null; + } + + @Override + public void removeConsumer(Consumer consumer) throws BrokerServiceException { + + } + + @Override + public void consumerFlow(Consumer consumer, int additionalNumberOfMessages) { + + } + + @Override + public CompletableFuture close(boolean disconnectClients, + Optional assignedBrokerLookupData) { + return null; + } + + @Override + public CompletableFuture disconnectActiveConsumers(boolean isResetCursor) { + return null; + } + + @Override + public CompletableFuture disconnectAllConsumers(boolean isResetCursor, + Optional assignedBrokerLookupData) { + return null; + } + + @Override + public void reset() { + + } + + @Override + public void redeliverUnacknowledgedMessages(Consumer consumer, long consumerEpoch) { + + } + + @Override + public void redeliverUnacknowledgedMessages(Consumer consumer, List positions) { + + } + + @Override + public void addUnAckedMessages(int unAckMessages) { + + } + + @Override + public RedeliveryTracker getRedeliveryTracker() { + return null; + } + + private static class MockBrokerService extends BrokerService { + + public MockBrokerService(PulsarService pulsarService) throws Exception { + super(pulsarService, null); + } + + public static MockBrokerService create() { + ServiceConfiguration config = new ServiceConfiguration(); + config.setClusterName("use"); + config.setWebServicePort(Optional.of(0)); + config.setMetadataStoreUrl("zk:127.0.0.1:2181"); + // config.setBrokerShutdownTimeoutMs(0L); + // config.setLoadBalancerOverrideBrokerNicSpeedGbps(Optional.of(1.0d)); + config.setBrokerServicePort(Optional.of(0)); + config.setBrokerServicePortTls(Optional.of(0)); + config.setWebServicePortTls(Optional.of(0)); + config.setAdvertisedAddress("localhost"); + try (PulsarService pulsarService = new PulsarService(config)) { + return new MockBrokerService(pulsarService); + } catch (Exception e) { + log.warn("Failed to create MockBrokerService", e); + e.printStackTrace(); + } + return null; + } + } + + private static class MockSubscription implements Subscription { + private final Topic topic; + + public MockSubscription(Topic topic) { + this.topic = topic; + } + + @Override + public BrokerInterceptor interceptor() { + return null; + } + + @Override + public Topic getTopic() { + return topic; + } + + @Override + public String getName() { + return "mock-subscription"; + } + + @Override + public CompletableFuture addConsumer(Consumer consumer) { + return null; + } + + @Override + public void removeConsumer(Consumer consumer, boolean isResetCursor) throws BrokerServiceException { + + } + + @Override + public void consumerFlow(Consumer consumer, int additionalNumberOfMessages) { + + } + + @Override + public void acknowledgeMessage(List positions, CommandAck.AckType ackType, + Map properties) { + + } + + @Override + public String getTopicName() { + return topic.getName(); + } + + @Override + public boolean isReplicated() { + return false; + } + + @Override + public Dispatcher getDispatcher() { + return null; + } + + @Override + public long getNumberOfEntriesInBacklog(boolean getPreciseBacklog) { + return 0; + } + + @Override + public List getConsumers() { + return List.of(); + } + + @Override + public CompletableFuture delete() { + return null; + } + + @Override + public CompletableFuture deleteForcefully() { + return null; + } + + @Override + public CompletableFuture disconnect(Optional assignedBrokerLookupData) { + return null; + } + + @Override + public CompletableFuture close(boolean disconnectConsumers, + Optional assignedBrokerLookupData) { + return null; + } + + @Override + public CompletableFuture doUnsubscribe(Consumer consumer) { + return null; + } + + @Override + public CompletableFuture doUnsubscribe(Consumer consumer, boolean forcefully) { + return null; + } + + @Override + public CompletableFuture clearBacklog() { + return null; + } + + @Override + public CompletableFuture skipMessages(int numMessagesToSkip) { + return null; + } + + @Override + public CompletableFuture resetCursor(long timestamp) { + return null; + } + + @Override + public CompletableFuture resetCursor(Position position) { + return null; + } + + @Override + public CompletableFuture peekNthMessage(int messagePosition) { + return null; + } + + @Override + public void redeliverUnacknowledgedMessages(Consumer consumer, long consumerEpoch) { + + } + + @Override + public void redeliverUnacknowledgedMessages(Consumer consumer, List positions) { + + } + + @Override + public void markTopicWithBatchMessagePublished() { + + } + + @Override + public double getExpiredMessageRate() { + return 0; + } + + @Override + public CommandSubscribe.SubType getType() { + return null; + } + + @Override + public String getTypeString() { + return ""; + } + + @Override + public void addUnAckedMessages(int unAckMessages) { + + } + + @Override + public Map getSubscriptionProperties() { + return Map.of(); + } + + @Override + public CompletableFuture updateSubscriptionProperties(Map subscriptionProperties) { + return null; + } + + @Override + public boolean isSubscriptionMigrated() { + return false; + } + + @Override + public CompletableFuture endTxn(long txnidMostBits, long txnidLeastBits, + int txnAction, long lowWaterMark) { + return null; + } + + @Override + public CompletableFuture analyzeBacklog(Optional position) { + return null; + } + + @Override + public boolean expireMessages(Position position) { + return false; + } + + @Override + public boolean expireMessages(int messageTTLInSeconds) { + return false; + } + + @Override + public CompletableFuture expireMessagesAsync(int messageTTLInSeconds) { + return null; + } + } + + // 简单的Mock Topic实现 + private static class MockTopic implements Topic { + @Override + public CompletableFuture initialize() { + return null; + } + + @Override + public void publishMessage(ByteBuf headersAndPayload, PublishContext callback) { + + } + + @Override + public CompletableFuture> addProducer(Producer producer, + CompletableFuture producerQueuedFuture) { + return null; + } + + @Override + public void removeProducer(Producer producer) { + + } + + @Override + public CompletableFuture checkIfTransactionBufferRecoverCompletely() { + return null; + } + + @Override + public void recordAddLatency(long latency, TimeUnit unit) { + + } + + @Override + public long increasePublishLimitedTimes() { + return 0; + } + + @Override + public CompletableFuture subscribe(TransportCnx cnx, String subscriptionName, + long consumerId, CommandSubscribe.SubType subType, + int priorityLevel, String consumerName, boolean isDurable, + MessageId startMessageId, Map metadata, + boolean readCompacted, + CommandSubscribe.InitialPosition initialPosition, + long startMessageRollbackDurationSec, + boolean replicateSubscriptionState, KeySharedMeta keySharedMeta) { + return null; + } + + @Override + public CompletableFuture subscribe(SubscriptionOption option) { + return null; + } + + @Override + public CompletableFuture createSubscription(String subscriptionName, + CommandSubscribe.InitialPosition initialPosition, + boolean replicateSubscriptionState, + Map properties) { + return null; + } + + @Override + public CompletableFuture unsubscribe(String subName) { + return null; + } + + @Override + public Map getSubscriptions() { + return Map.of(); + } + + @Override + public CompletableFuture delete() { + return null; + } + + @Override + public Map getProducers() { + return Map.of(); + } + + @Override + public String getName() { + return "mock-topic"; + } + + @Override + public CompletableFuture checkReplication() { + return null; + } + + @Override + public CompletableFuture close(boolean closeWithoutWaitingClientDisconnect) { + return null; + } + + @Override + public CompletableFuture close(boolean disconnectClients, boolean closeWithoutWaitingClientDisconnect) { + return null; + } + + @Override + public void checkGC() { + + } + + @Override + public CompletableFuture checkClusterMigration() { + return null; + } + + @Override + public void checkInactiveSubscriptions() { + + } + + @Override + public void checkBackloggedCursors() { + + } + + @Override + public void checkCursorsToCacheEntries() { + + } + + @Override + public void checkDeduplicationSnapshot() { + + } + + @Override + public void checkMessageExpiry() { + + } + + @Override + public void checkMessageDeduplicationInfo() { + + } + + @Override + public void incrementPublishCount(Producer producer, int numOfMessages, long msgSizeInBytes) { + + } + + @Override + public boolean shouldProducerMigrate() { + return false; + } + + @Override + public boolean isReplicationBacklogExist() { + return false; + } + + @Override + public CompletableFuture onPoliciesUpdate(Policies data) { + return null; + } + + @Override + public CompletableFuture checkBacklogQuotaExceeded(String producerName, + BacklogQuota.BacklogQuotaType backlogQuotaType) { + return null; + } + + @Override + public boolean isEncryptionRequired() { + return false; + } + + @Override + public boolean getSchemaValidationEnforced() { + return false; + } + + @Override + public boolean isReplicated() { + return false; + } + + @Override + public boolean isShadowReplicated() { + return false; + } + + @Override + public EntryFilters getEntryFiltersPolicy() { + return null; + } + + @Override + public List getEntryFilters() { + return List.of(); + } + + @Override + public BacklogQuota getBacklogQuota(BacklogQuota.BacklogQuotaType backlogQuotaType) { + return null; + } + + @Override + public long getBestEffortOldestUnacknowledgedMessageAgeSeconds() { + return 0; + } + + @Override + public void updateRates(NamespaceStats nsStats, NamespaceBundleStats currentBundleStats, + StatsOutputStream topicStatsStream, + ClusterReplicationMetrics clusterReplicationMetrics, + String namespaceName, boolean hydratePublishers) { + + } + + @Override + public Subscription getSubscription(String subscription) { + return null; + } + + @Override + public Map getReplicators() { + return Map.of(); + } + + @Override + public Map getShadowReplicators() { + return Map.of(); + } + + @Override + public TopicStatsImpl getStats(boolean getPreciseBacklog, boolean subscriptionBacklogSize, + boolean getEarliestTimeInBacklog) { + return null; + } + + @Override + public TopicStatsImpl getStats(GetStatsOptions getStatsOptions) { + return null; + } + + @Override + public CompletableFuture asyncGetStats(boolean getPreciseBacklog, + boolean subscriptionBacklogSize, + boolean getEarliestTimeInBacklog) { + return null; + } + + @Override + public CompletableFuture asyncGetStats(GetStatsOptions getStatsOptions) { + return null; + } + + @Override + public CompletableFuture getInternalStats(boolean includeLedgerMetadata) { + return null; + } + + @Override + public Position getLastPosition() { + return null; + } + + @Override + public CompletableFuture getLastMessageId() { + return null; + } + + @Override + public CompletableFuture hasSchema() { + return null; + } + + @Override + public CompletableFuture addSchema(SchemaData schema) { + return null; + } + + @Override + public CompletableFuture deleteSchema() { + return null; + } + + @Override + public CompletableFuture checkSchemaCompatibleForConsumer(SchemaData schema) { + return null; + } + + @Override + public CompletableFuture addSchemaIfIdleOrCheckCompatible(SchemaData schema) { + return null; + } + + @Override + public CompletableFuture deleteForcefully() { + return null; + } + + @Override + public boolean isPersistent() { + return false; + } + + @Override + public boolean isTransferring() { + return false; + } + + @Override + public void publishTxnMessage(TxnID txnID, ByteBuf headersAndPayload, PublishContext publishContext) { + + } + + @Override + public CompletableFuture endTxn(TxnID txnID, int txnAction, long lowWaterMark) { + return null; + } + + @Override + public CompletableFuture truncate() { + return null; + } + + @Override + public BrokerService getBrokerService() { + return MockBrokerService.create(); + } + + @Override + public HierarchyTopicPolicies getHierarchyTopicPolicies() { + return null; + } + + @Override + public TopicAttributes getTopicAttributes() { + return null; + } + } +} diff --git a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java new file mode 100644 index 0000000000000..fb22ebcf17bb1 --- /dev/null +++ b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java @@ -0,0 +1,254 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker.delayed.bucket; + +import io.netty.util.HashedWheelTimer; +import io.netty.util.Timer; +import io.netty.util.concurrent.DefaultThreadFactory; +import java.time.Clock; +import java.util.NavigableSet; +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; +import org.apache.bookkeeper.mledger.Position; +import org.apache.pulsar.broker.MockPersistentDispatcher; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +/** + * Enhanced JMH Benchmarks for BucketDelayedDeliveryTracker with ReentrantReadWriteLock. + * This benchmark tests the performance improvements made by transitioning from + * StampedLock to ReentrantReadWriteLock for fine-grained concurrency control. + *

+ * Run with: mvn exec:java -Dexec.mainClass="org.openjdk.jmh.Main" + * -Dexec.args="BucketDelayedDeliveryTrackerBenchmark" + */ +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.SECONDS) +@State(Scope.Benchmark) +@Warmup(iterations = 3, time = 2) +@Measurement(iterations = 5, time = 3) +@Fork(1) +public class BucketDelayedDeliveryTrackerBenchmark { + + @Param({"90_10", "80_20", "70_30", "50_50"}) + public String readWriteRatio; + + @Param({"1000", "5000", "10000"}) + public int initialMessages; + + private BucketDelayedDeliveryTracker tracker; + private Timer timer; + private MockBucketSnapshotStorage storage; + private MockPersistentDispatcher dispatcher; + private AtomicLong messageIdGenerator; + + @Setup(Level.Trial) + public void setup() throws Exception { + setupMockComponents(); + createTracker(); + preloadMessages(); + messageIdGenerator = new AtomicLong(initialMessages + 1); + } + + @TearDown(Level.Trial) + public void tearDown() throws Exception { + if (tracker != null) { + tracker.close(); + } + if (timer != null) { + timer.stop(); + } + } + + private void setupMockComponents() throws Exception { + timer = new HashedWheelTimer(new DefaultThreadFactory("test-delayed-delivery"), 100, TimeUnit.MILLISECONDS); + storage = new MockBucketSnapshotStorage(); + dispatcher = MockPersistentDispatcher.create(); + } + + private void createTracker() throws Exception { + tracker = new BucketDelayedDeliveryTracker( + dispatcher, timer, 1000, Clock.systemUTC(), true, storage, + 20, 1000, 100, 50 + ); + } + + private void preloadMessages() { + // Preload messages to create realistic test conditions + long baseTime = System.currentTimeMillis() + 10000; // Future delivery + for (int i = 1; i <= initialMessages; i++) { + tracker.addMessage(i, i, baseTime + i * 1000); + } + } + + // ============================================================================= + // READ-WRITE RATIO BENCHMARKS + // ============================================================================= + + @Benchmark + public boolean benchmarkMixedOperations() { + String[] parts = readWriteRatio.split("_"); + int readPercentage = Integer.parseInt(parts[0]); + + if (ThreadLocalRandom.current().nextInt(100) < readPercentage) { + // Read operations + return performReadOperation(); + } else { + // Write operations + return performWriteOperation(); + } + } + + private boolean performReadOperation() { + int operation = ThreadLocalRandom.current().nextInt(3); + switch (operation) { + case 0: + // containsMessage + long ledgerId = ThreadLocalRandom.current().nextLong(1, initialMessages + 100); + long entryId = ThreadLocalRandom.current().nextLong(1, 1000); + return tracker.containsMessage(ledgerId, entryId); + case 1: + // nextDeliveryTime + try { + tracker.nextDeliveryTime(); + return true; + } catch (Exception e) { + return false; + } + case 2: + // getNumberOfDelayedMessages + long count = tracker.getNumberOfDelayedMessages(); + return count >= 0; + default: + return false; + } + } + + private boolean performWriteOperation() { + long id = messageIdGenerator.getAndIncrement(); + long deliverAt = System.currentTimeMillis() + ThreadLocalRandom.current().nextLong(5000, 30000); + return tracker.addMessage(id, id % 1000, deliverAt); + } + + // ============================================================================= + // SPECIFIC OPERATION BENCHMARKS + // ============================================================================= + + @Benchmark + @Threads(8) + public boolean benchmarkConcurrentContainsMessage() { + long ledgerId = ThreadLocalRandom.current().nextLong(1, initialMessages + 100); + long entryId = ThreadLocalRandom.current().nextLong(1, 1000); + return tracker.containsMessage(ledgerId, entryId); + } + + @Benchmark + @Threads(4) + public boolean benchmarkConcurrentAddMessage() { + long id = messageIdGenerator.getAndIncrement(); + long deliverAt = System.currentTimeMillis() + ThreadLocalRandom.current().nextLong(10000, 60000); + return tracker.addMessage(id, id % 1000, deliverAt); + } + + @Benchmark + @Threads(2) + public NavigableSet benchmarkConcurrentGetScheduledMessages() { + // Create some messages ready for delivery + long currentTime = System.currentTimeMillis(); + for (int i = 0; i < 5; i++) { + long id = messageIdGenerator.getAndIncrement(); + tracker.addMessage(id, id % 100, currentTime - 1000); + } + return tracker.getScheduledMessages(10); + } + + @Benchmark + @Threads(16) + public long benchmarkConcurrentNextDeliveryTime() { + try { + return tracker.nextDeliveryTime(); + } catch (Exception e) { + return -1; + } + } + + @Benchmark + @Threads(1) + public long benchmarkGetNumberOfDelayedMessages() { + return tracker.getNumberOfDelayedMessages(); + } + + // ============================================================================= + // HIGH CONTENTION SCENARIOS + // ============================================================================= + + @Benchmark + @Threads(32) + public boolean benchmarkHighContentionMixedOperations() { + return benchmarkMixedOperations(); + } + + @Benchmark + @Threads(16) + public boolean benchmarkContentionReads() { + return performReadOperation(); + } + + @Benchmark + @Threads(8) + public boolean benchmarkContentionWrites() { + return performWriteOperation(); + } + + // ============================================================================= + // THROUGHPUT BENCHMARKS + // ============================================================================= + + @Benchmark + @Threads(1) + public boolean benchmarkSingleThreadedThroughput() { + return benchmarkMixedOperations(); + } + + @Benchmark + @Threads(4) + public boolean benchmarkMediumConcurrencyThroughput() { + return benchmarkMixedOperations(); + } + + @Benchmark + @Threads(8) + public boolean benchmarkHighConcurrencyThroughput() { + return benchmarkMixedOperations(); + } + +} diff --git a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerSimpleBenchmark.java b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerSimpleBenchmark.java deleted file mode 100644 index 985e714d54d1d..0000000000000 --- a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerSimpleBenchmark.java +++ /dev/null @@ -1,408 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.pulsar.broker.delayed.bucket; - -import java.util.concurrent.ThreadLocalRandom; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.locks.StampedLock; -import org.openjdk.jmh.annotations.Benchmark; -import org.openjdk.jmh.annotations.BenchmarkMode; -import org.openjdk.jmh.annotations.Fork; -import org.openjdk.jmh.annotations.Level; -import org.openjdk.jmh.annotations.Measurement; -import org.openjdk.jmh.annotations.Mode; -import org.openjdk.jmh.annotations.OutputTimeUnit; -import org.openjdk.jmh.annotations.Param; -import org.openjdk.jmh.annotations.Scope; -import org.openjdk.jmh.annotations.Setup; -import org.openjdk.jmh.annotations.State; -import org.openjdk.jmh.annotations.TearDown; -import org.openjdk.jmh.annotations.Threads; -import org.openjdk.jmh.annotations.Warmup; - -/** - * Simplified JMH Benchmarks for BucketDelayedDeliveryTracker thread safety improvements. - * This benchmark focuses on the core StampedLock optimistic read performance without - * complex dependencies on the full BucketDelayedDeliveryTracker implementation. - * Run with: mvn exec:java -Dexec.mainClass="org.openjdk.jmh.Main" - * -Dexec.args="BucketDelayedDeliveryTrackerSimpleBenchmark" - */ -@BenchmarkMode(Mode.Throughput) -@OutputTimeUnit(TimeUnit.SECONDS) -@State(Scope.Benchmark) -@Warmup(iterations = 5, time = 1) -@Measurement(iterations = 5, time = 1) -@Fork(1) -public class BucketDelayedDeliveryTrackerSimpleBenchmark { - - @Param({"1", "2", "4", "8", "16"}) - public int threadCount; - - private StampedLock stampedLock; - private boolean testData = true; - private volatile long counter = 0; - - @Setup(Level.Trial) - public void setup() throws Exception { - stampedLock = new StampedLock(); - } - - @TearDown(Level.Trial) - public void tearDown() throws Exception { - // Cleanup if needed - } - - // ============================================================================= - // STAMPED LOCK OPTIMISTIC READ BENCHMARKS - // ============================================================================= - - @Benchmark - @Threads(1) - public boolean benchmarkOptimisticReadSingleThreaded() { - // Simulate optimistic read like in containsMessage() - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; // Simulate reading shared data - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } - - @Benchmark - @Threads(2) - public boolean benchmarkOptimisticReadMultiThreaded() { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } - - @Benchmark - @Threads(8) - public boolean benchmarkOptimisticReadHighConcurrency() { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } - - @Benchmark - @Threads(16) - public boolean benchmarkOptimisticReadExtremeConcurrency() { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } - - // ============================================================================= - // READ:WRITE RATIO BENCHMARKS (as requested) - // ============================================================================= - - @Benchmark - @Threads(4) - public boolean benchmarkReadWrite10_90() { - // 10:90 read:write ratio simulation - if (ThreadLocalRandom.current().nextInt(100) < 10) { - // Read operation - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } else { - // Write operation - long stamp = stampedLock.writeLock(); - try { - testData = !testData; - counter++; - return testData; - } finally { - stampedLock.unlockWrite(stamp); - } - } - } - - @Benchmark - @Threads(4) - public boolean benchmarkReadWrite20_80() { - // 20:80 read:write ratio - if (ThreadLocalRandom.current().nextInt(100) < 20) { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } else { - long stamp = stampedLock.writeLock(); - try { - testData = !testData; - counter++; - return testData; - } finally { - stampedLock.unlockWrite(stamp); - } - } - } - - @Benchmark - @Threads(4) - public boolean benchmarkReadWrite40_60() { - // 40:60 read:write ratio - if (ThreadLocalRandom.current().nextInt(100) < 40) { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } else { - long stamp = stampedLock.writeLock(); - try { - testData = !testData; - counter++; - return testData; - } finally { - stampedLock.unlockWrite(stamp); - } - } - } - - @Benchmark - @Threads(4) - public boolean benchmarkReadWrite50_50() { - // 50:50 read:write ratio - if (ThreadLocalRandom.current().nextInt(100) < 50) { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } else { - long stamp = stampedLock.writeLock(); - try { - testData = !testData; - counter++; - return testData; - } finally { - stampedLock.unlockWrite(stamp); - } - } - } - - @Benchmark - @Threads(4) - public boolean benchmarkReadWrite60_40() { - // 60:40 read:write ratio - if (ThreadLocalRandom.current().nextInt(100) < 60) { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } else { - long stamp = stampedLock.writeLock(); - try { - testData = !testData; - counter++; - return testData; - } finally { - stampedLock.unlockWrite(stamp); - } - } - } - - @Benchmark - @Threads(4) - public boolean benchmarkReadWrite80_20() { - // 80:20 read:write ratio - if (ThreadLocalRandom.current().nextInt(100) < 80) { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } else { - long stamp = stampedLock.writeLock(); - try { - testData = !testData; - counter++; - return testData; - } finally { - stampedLock.unlockWrite(stamp); - } - } - } - - @Benchmark - @Threads(4) - public boolean benchmarkReadWrite90_10() { - // 90:10 read:write ratio - most realistic for production - if (ThreadLocalRandom.current().nextInt(100) < 90) { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } else { - long stamp = stampedLock.writeLock(); - try { - testData = !testData; - counter++; - return testData; - } finally { - stampedLock.unlockWrite(stamp); - } - } - } - - // ============================================================================= - // HIGH CONCURRENCY SCENARIOS - // ============================================================================= - - @Benchmark - @Threads(8) - public boolean benchmarkReadWrite90_10_HighConcurrency() { - // 90:10 read:write ratio with high concurrency - if (ThreadLocalRandom.current().nextInt(100) < 90) { - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } else { - long stamp = stampedLock.writeLock(); - try { - testData = !testData; - counter++; - return testData; - } finally { - stampedLock.unlockWrite(stamp); - } - } - } - - @Benchmark - @Threads(16) - public boolean benchmarkOptimisticReadContention() { - // High contention scenario to test optimistic read fallback behavior - long stamp = stampedLock.tryOptimisticRead(); - boolean result = testData; - - // Simulate some computation - if (ThreadLocalRandom.current().nextInt(1000) == 0) { - Thread.yield(); // Occasionally yield to increase contention - } - - if (!stampedLock.validate(stamp)) { - stamp = stampedLock.readLock(); - try { - result = testData; - } finally { - stampedLock.unlockRead(stamp); - } - } - return result; - } -} \ No newline at end of file diff --git a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/MockBucketSnapshotStorage.java b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/MockBucketSnapshotStorage.java new file mode 100644 index 0000000000000..d220658fb2c6d --- /dev/null +++ b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/MockBucketSnapshotStorage.java @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker.delayed.bucket; + +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicLong; +import org.apache.pulsar.broker.delayed.proto.SnapshotMetadata; +import org.apache.pulsar.broker.delayed.proto.SnapshotSegment; + +public class MockBucketSnapshotStorage implements BucketSnapshotStorage { + + private final AtomicLong idGenerator = new AtomicLong(1); + private final Map snapshots = new ConcurrentHashMap<>(); + + @Override + public CompletableFuture createBucketSnapshot(SnapshotMetadata snapshotMetadata, + List bucketSnapshotSegments, + String bucketKey, String topicName, String cursorName) { + long id = idGenerator.getAndIncrement(); + snapshots.put(id, snapshotMetadata); + return CompletableFuture.completedFuture(id); + } + + @Override + public CompletableFuture getBucketSnapshotMetadata(long bucketId) { + SnapshotMetadata metadata = snapshots.get(bucketId); + return CompletableFuture.completedFuture(metadata); + } + + @Override + public CompletableFuture> getBucketSnapshotSegment(long bucketId, + long firstSegmentEntryId, + long lastSegmentEntryId) { + return CompletableFuture.completedFuture(Collections.emptyList()); + } + + @Override + public CompletableFuture getBucketSnapshotLength(long bucketId) { + return CompletableFuture.completedFuture(0L); + } + + @Override + public CompletableFuture deleteBucketSnapshot(long bucketId) { + snapshots.remove(bucketId); + return CompletableFuture.completedFuture(null); + } + + @Override + public void start() throws Exception { + // No-op + } + + @Override + public void close() throws Exception { + snapshots.clear(); + } +} diff --git a/microbench/src/main/java/org/apache/pulsar/broker/package-info.java b/microbench/src/main/java/org/apache/pulsar/broker/package-info.java new file mode 100644 index 0000000000000..bb5c91c6d31ac --- /dev/null +++ b/microbench/src/main/java/org/apache/pulsar/broker/package-info.java @@ -0,0 +1,27 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Microbenchmarks for delayed message delivery bucket implementation. + * + *

This package contains JMH benchmarks for testing the performance + * characteristics of the BucketDelayedDeliveryTracker, particularly + * focusing on thread safety improvements with StampedLock optimistic reads. + */ +package org.apache.pulsar.broker; \ No newline at end of file From 591c6d5552ac49a827d55db236187de712b0d74b Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Sat, 20 Sep 2025 21:59:34 +0800 Subject: [PATCH 11/16] refactor(test): Make MockManagedCursor class public for testing --- .../apache/bookkeeper/mledger/impl/MockManagedCursor.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/microbench/src/main/java/org/apache/bookkeeper/mledger/impl/MockManagedCursor.java b/microbench/src/main/java/org/apache/bookkeeper/mledger/impl/MockManagedCursor.java index af602a8a9fe50..927b4a1c9299e 100644 --- a/microbench/src/main/java/org/apache/bookkeeper/mledger/impl/MockManagedCursor.java +++ b/microbench/src/main/java/org/apache/bookkeeper/mledger/impl/MockManagedCursor.java @@ -36,7 +36,7 @@ import org.apache.bookkeeper.mledger.Position; import org.apache.pulsar.common.policies.data.ManagedLedgerInternalStats; -class MockManagedCursor implements ManagedCursor { +public class MockManagedCursor implements ManagedCursor { ActiveManagedCursorContainer container; Position markDeletePosition; Position readPosition; @@ -58,7 +58,8 @@ class MockManagedCursor implements ManagedCursor { this.durable = durable; } - static MockManagedCursor createCursor(ActiveManagedCursorContainer container, String name, Position position) { + public static MockManagedCursor createCursor(ActiveManagedCursorContainer container, String name, + Position position) { return new MockManagedCursor(container, name, position, position, false, true); } From fe5d98d51befd8c9dee64f104a993be79cfc0ccc Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Mon, 22 Sep 2025 21:12:17 +0800 Subject: [PATCH 12/16] feat(delayed): introduce DelayedDeliveryContext abstraction for tracker decoupling --- .../broker/MockPersistentDispatcher.java | 860 ------------------ ...BucketDelayedDeliveryTrackerBenchmark.java | 15 +- .../AbstractDelayedDeliveryTracker.java | 112 ++- .../BucketDelayedDeliveryTrackerFactory.java | 13 +- .../delayed/DelayedDeliveryContext.java | 30 + .../DispatcherDelayedDeliveryContext.java | 48 + .../InMemoryDelayedDeliveryTracker.java | 37 +- ...InMemoryDelayedDeliveryTrackerFactory.java | 11 +- .../delayed/NoopDelayedDeliveryContext.java | 54 ++ .../bucket/BucketDelayedDeliveryTracker.java | 98 +- .../BucketDelayedDeliveryTrackerTest.java | 2 +- 11 files changed, 332 insertions(+), 948 deletions(-) delete mode 100644 microbench/src/main/java/org/apache/pulsar/broker/MockPersistentDispatcher.java create mode 100644 pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DelayedDeliveryContext.java create mode 100644 pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DispatcherDelayedDeliveryContext.java create mode 100644 pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/NoopDelayedDeliveryContext.java diff --git a/microbench/src/main/java/org/apache/pulsar/broker/MockPersistentDispatcher.java b/microbench/src/main/java/org/apache/pulsar/broker/MockPersistentDispatcher.java deleted file mode 100644 index 198091c1a42be..0000000000000 --- a/microbench/src/main/java/org/apache/pulsar/broker/MockPersistentDispatcher.java +++ /dev/null @@ -1,860 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.pulsar.broker; - -import io.netty.buffer.ByteBuf; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.TimeUnit; -import lombok.extern.slf4j.Slf4j; -import org.apache.bookkeeper.mledger.Entry; -import org.apache.bookkeeper.mledger.ManagedCursor; -import org.apache.bookkeeper.mledger.ManagedLedgerException; -import org.apache.bookkeeper.mledger.Position; -import org.apache.bookkeeper.mledger.PositionFactory; -import org.apache.bookkeeper.mledger.impl.ActiveManagedCursorContainerImpl; -import org.apache.bookkeeper.mledger.impl.MockManagedCursor; -import org.apache.pulsar.broker.intercept.BrokerInterceptor; -import org.apache.pulsar.broker.loadbalance.extensions.data.BrokerLookupData; -import org.apache.pulsar.broker.service.AnalyzeBacklogResult; -import org.apache.pulsar.broker.service.BrokerService; -import org.apache.pulsar.broker.service.BrokerServiceException; -import org.apache.pulsar.broker.service.Consumer; -import org.apache.pulsar.broker.service.Dispatcher; -import org.apache.pulsar.broker.service.GetStatsOptions; -import org.apache.pulsar.broker.service.Producer; -import org.apache.pulsar.broker.service.RedeliveryTracker; -import org.apache.pulsar.broker.service.Replicator; -import org.apache.pulsar.broker.service.Subscription; -import org.apache.pulsar.broker.service.SubscriptionOption; -import org.apache.pulsar.broker.service.Topic; -import org.apache.pulsar.broker.service.TopicAttributes; -import org.apache.pulsar.broker.service.TransportCnx; -import org.apache.pulsar.broker.service.persistent.AbstractPersistentDispatcherMultipleConsumers; -import org.apache.pulsar.broker.service.plugin.EntryFilter; -import org.apache.pulsar.broker.stats.ClusterReplicationMetrics; -import org.apache.pulsar.broker.stats.NamespaceStats; -import org.apache.pulsar.client.api.MessageId; -import org.apache.pulsar.client.api.transaction.TxnID; -import org.apache.pulsar.common.api.proto.CommandAck; -import org.apache.pulsar.common.api.proto.CommandSubscribe; -import org.apache.pulsar.common.api.proto.KeySharedMeta; -import org.apache.pulsar.common.policies.data.BacklogQuota; -import org.apache.pulsar.common.policies.data.EntryFilters; -import org.apache.pulsar.common.policies.data.HierarchyTopicPolicies; -import org.apache.pulsar.common.policies.data.PersistentTopicInternalStats; -import org.apache.pulsar.common.policies.data.Policies; -import org.apache.pulsar.common.policies.data.stats.TopicMetricBean; -import org.apache.pulsar.common.policies.data.stats.TopicStatsImpl; -import org.apache.pulsar.common.protocol.schema.SchemaData; -import org.apache.pulsar.common.protocol.schema.SchemaVersion; -import org.apache.pulsar.common.util.Codec; -import org.apache.pulsar.policies.data.loadbalancer.NamespaceBundleStats; -import org.apache.pulsar.utils.StatsOutputStream; - -@Slf4j -public class MockPersistentDispatcher extends AbstractPersistentDispatcherMultipleConsumers { - - private final MockManagedCursor cursor; - private final MockSubscription subscription; - private final MockTopic topic; - - private MockPersistentDispatcher(MockTopic topic, MockSubscription subscription, - MockManagedCursor cursor) { - super(subscription, new ServiceConfiguration()); - this.topic = topic; - this.subscription = subscription; - this.cursor = cursor; - } - - public static MockPersistentDispatcher create() throws Exception { - // LocalBookkeeperEnsemble bkEnsemble = new LocalBookkeeperEnsemble(3, 0, () -> 0); - // bkEnsemble.start(); - // - // // Start broker - // ServiceConfiguration config = new ServiceConfiguration(); - // config.setClusterName("use"); - // config.setWebServicePort(Optional.of(0)); - // config.setMetadataStoreUrl("zk:127.0.0.1:" + bkEnsemble.getZookeeperPort()); - // config.setBrokerShutdownTimeoutMs(0L); - // config.setLoadBalancerOverrideBrokerNicSpeedGbps(Optional.of(1.0d)); - // config.setBrokerServicePort(Optional.of(0)); - // config.setLoadManagerClassName(SimpleLoadManagerImpl.class.getName()); - // config.setBrokerServicePortTls(Optional.of(0)); - // config.setWebServicePortTls(Optional.of(0)); - // config.setAdvertisedAddress("localhost"); - // - // @Cleanup - // PulsarService pulsarService = new PulsarService(config); - // pulsarService.start(); - // - // ActiveManagedCursorContainerImpl container = new ActiveManagedCursorContainerImpl(); - // MockManagedCursor cursor = MockManagedCursor.createCursor(container, "cursor", - // PositionFactory.create(0, 1)); - // - // - // - // TopicName topicName = TopicName.get("persistent://pulsar/default/ns/topic"); - // PersistentTopic topic = new PersistentTopic(topicName.toString(), cursor.getManagedLedger(), - // pulsarService.getBrokerService()); - // - // PersistentSubscription subscription = new PersistentSubscription(topic, "sub", cursor, false); - // - // return new MockPersistentDispatcher(topic, subscription, cursor); - - - ActiveManagedCursorContainerImpl container = new ActiveManagedCursorContainerImpl(); - MockManagedCursor cursor = MockManagedCursor.createCursor(container, "test-cursor", - PositionFactory.create(0, 0)); - - MockTopic topic = new MockTopic(); - MockSubscription subscription = new MockSubscription(topic); - - return new MockPersistentDispatcher(topic, subscription, cursor); - } - - @Override - public void unBlockDispatcherOnUnackedMsgs() { - - } - - @Override - public void readMoreEntriesAsync() { - - } - - @Override - protected boolean isConsumersExceededOnSubscription() { - return false; - } - - @Override - protected void reScheduleRead() { - - } - - @Override - public String getName() { - return topic.getName() + " / " + Codec.decode(cursor.getName()); - } - - @Override - public boolean isBlockedDispatcherOnUnackedMsgs() { - return false; - } - - @Override - public int getTotalUnackedMessages() { - return 0; - } - - @Override - public void blockDispatcherOnUnackedMsgs() { - - } - - @Override - public long getNumberOfMessagesInReplay() { - return 0; - } - - @Override - public boolean isHavePendingRead() { - return false; - } - - @Override - public boolean isHavePendingReplayRead() { - return false; - } - - @Override - public ManagedCursor getCursor() { - return cursor; - } - - @Override - public Topic getTopic() { - return topic; - } - - @Override - public Subscription getSubscription() { - return subscription; - } - - @Override - public long getDelayedTrackerMemoryUsage() { - return 0; - } - - @Override - public Map getBucketDelayedIndexStats() { - return Map.of(); - } - - @Override - public boolean isClassic() { - return false; - } - - @Override - public void readEntriesComplete(List entries, Object ctx) { - - } - - @Override - public void readEntriesFailed(ManagedLedgerException exception, Object ctx) { - - } - - @Override - public boolean isConsumerAvailable(Consumer consumer) { - return false; - } - - @Override - public CompletableFuture addConsumer(Consumer consumer) { - return null; - } - - @Override - public void removeConsumer(Consumer consumer) throws BrokerServiceException { - - } - - @Override - public void consumerFlow(Consumer consumer, int additionalNumberOfMessages) { - - } - - @Override - public CompletableFuture close(boolean disconnectClients, - Optional assignedBrokerLookupData) { - return null; - } - - @Override - public CompletableFuture disconnectActiveConsumers(boolean isResetCursor) { - return null; - } - - @Override - public CompletableFuture disconnectAllConsumers(boolean isResetCursor, - Optional assignedBrokerLookupData) { - return null; - } - - @Override - public void reset() { - - } - - @Override - public void redeliverUnacknowledgedMessages(Consumer consumer, long consumerEpoch) { - - } - - @Override - public void redeliverUnacknowledgedMessages(Consumer consumer, List positions) { - - } - - @Override - public void addUnAckedMessages(int unAckMessages) { - - } - - @Override - public RedeliveryTracker getRedeliveryTracker() { - return null; - } - - private static class MockBrokerService extends BrokerService { - - public MockBrokerService(PulsarService pulsarService) throws Exception { - super(pulsarService, null); - } - - public static MockBrokerService create() { - ServiceConfiguration config = new ServiceConfiguration(); - config.setClusterName("use"); - config.setWebServicePort(Optional.of(0)); - config.setMetadataStoreUrl("zk:127.0.0.1:2181"); - // config.setBrokerShutdownTimeoutMs(0L); - // config.setLoadBalancerOverrideBrokerNicSpeedGbps(Optional.of(1.0d)); - config.setBrokerServicePort(Optional.of(0)); - config.setBrokerServicePortTls(Optional.of(0)); - config.setWebServicePortTls(Optional.of(0)); - config.setAdvertisedAddress("localhost"); - try (PulsarService pulsarService = new PulsarService(config)) { - return new MockBrokerService(pulsarService); - } catch (Exception e) { - log.warn("Failed to create MockBrokerService", e); - e.printStackTrace(); - } - return null; - } - } - - private static class MockSubscription implements Subscription { - private final Topic topic; - - public MockSubscription(Topic topic) { - this.topic = topic; - } - - @Override - public BrokerInterceptor interceptor() { - return null; - } - - @Override - public Topic getTopic() { - return topic; - } - - @Override - public String getName() { - return "mock-subscription"; - } - - @Override - public CompletableFuture addConsumer(Consumer consumer) { - return null; - } - - @Override - public void removeConsumer(Consumer consumer, boolean isResetCursor) throws BrokerServiceException { - - } - - @Override - public void consumerFlow(Consumer consumer, int additionalNumberOfMessages) { - - } - - @Override - public void acknowledgeMessage(List positions, CommandAck.AckType ackType, - Map properties) { - - } - - @Override - public String getTopicName() { - return topic.getName(); - } - - @Override - public boolean isReplicated() { - return false; - } - - @Override - public Dispatcher getDispatcher() { - return null; - } - - @Override - public long getNumberOfEntriesInBacklog(boolean getPreciseBacklog) { - return 0; - } - - @Override - public List getConsumers() { - return List.of(); - } - - @Override - public CompletableFuture delete() { - return null; - } - - @Override - public CompletableFuture deleteForcefully() { - return null; - } - - @Override - public CompletableFuture disconnect(Optional assignedBrokerLookupData) { - return null; - } - - @Override - public CompletableFuture close(boolean disconnectConsumers, - Optional assignedBrokerLookupData) { - return null; - } - - @Override - public CompletableFuture doUnsubscribe(Consumer consumer) { - return null; - } - - @Override - public CompletableFuture doUnsubscribe(Consumer consumer, boolean forcefully) { - return null; - } - - @Override - public CompletableFuture clearBacklog() { - return null; - } - - @Override - public CompletableFuture skipMessages(int numMessagesToSkip) { - return null; - } - - @Override - public CompletableFuture resetCursor(long timestamp) { - return null; - } - - @Override - public CompletableFuture resetCursor(Position position) { - return null; - } - - @Override - public CompletableFuture peekNthMessage(int messagePosition) { - return null; - } - - @Override - public void redeliverUnacknowledgedMessages(Consumer consumer, long consumerEpoch) { - - } - - @Override - public void redeliverUnacknowledgedMessages(Consumer consumer, List positions) { - - } - - @Override - public void markTopicWithBatchMessagePublished() { - - } - - @Override - public double getExpiredMessageRate() { - return 0; - } - - @Override - public CommandSubscribe.SubType getType() { - return null; - } - - @Override - public String getTypeString() { - return ""; - } - - @Override - public void addUnAckedMessages(int unAckMessages) { - - } - - @Override - public Map getSubscriptionProperties() { - return Map.of(); - } - - @Override - public CompletableFuture updateSubscriptionProperties(Map subscriptionProperties) { - return null; - } - - @Override - public boolean isSubscriptionMigrated() { - return false; - } - - @Override - public CompletableFuture endTxn(long txnidMostBits, long txnidLeastBits, - int txnAction, long lowWaterMark) { - return null; - } - - @Override - public CompletableFuture analyzeBacklog(Optional position) { - return null; - } - - @Override - public boolean expireMessages(Position position) { - return false; - } - - @Override - public boolean expireMessages(int messageTTLInSeconds) { - return false; - } - - @Override - public CompletableFuture expireMessagesAsync(int messageTTLInSeconds) { - return null; - } - } - - // 简单的Mock Topic实现 - private static class MockTopic implements Topic { - @Override - public CompletableFuture initialize() { - return null; - } - - @Override - public void publishMessage(ByteBuf headersAndPayload, PublishContext callback) { - - } - - @Override - public CompletableFuture> addProducer(Producer producer, - CompletableFuture producerQueuedFuture) { - return null; - } - - @Override - public void removeProducer(Producer producer) { - - } - - @Override - public CompletableFuture checkIfTransactionBufferRecoverCompletely() { - return null; - } - - @Override - public void recordAddLatency(long latency, TimeUnit unit) { - - } - - @Override - public long increasePublishLimitedTimes() { - return 0; - } - - @Override - public CompletableFuture subscribe(TransportCnx cnx, String subscriptionName, - long consumerId, CommandSubscribe.SubType subType, - int priorityLevel, String consumerName, boolean isDurable, - MessageId startMessageId, Map metadata, - boolean readCompacted, - CommandSubscribe.InitialPosition initialPosition, - long startMessageRollbackDurationSec, - boolean replicateSubscriptionState, KeySharedMeta keySharedMeta) { - return null; - } - - @Override - public CompletableFuture subscribe(SubscriptionOption option) { - return null; - } - - @Override - public CompletableFuture createSubscription(String subscriptionName, - CommandSubscribe.InitialPosition initialPosition, - boolean replicateSubscriptionState, - Map properties) { - return null; - } - - @Override - public CompletableFuture unsubscribe(String subName) { - return null; - } - - @Override - public Map getSubscriptions() { - return Map.of(); - } - - @Override - public CompletableFuture delete() { - return null; - } - - @Override - public Map getProducers() { - return Map.of(); - } - - @Override - public String getName() { - return "mock-topic"; - } - - @Override - public CompletableFuture checkReplication() { - return null; - } - - @Override - public CompletableFuture close(boolean closeWithoutWaitingClientDisconnect) { - return null; - } - - @Override - public CompletableFuture close(boolean disconnectClients, boolean closeWithoutWaitingClientDisconnect) { - return null; - } - - @Override - public void checkGC() { - - } - - @Override - public CompletableFuture checkClusterMigration() { - return null; - } - - @Override - public void checkInactiveSubscriptions() { - - } - - @Override - public void checkBackloggedCursors() { - - } - - @Override - public void checkCursorsToCacheEntries() { - - } - - @Override - public void checkDeduplicationSnapshot() { - - } - - @Override - public void checkMessageExpiry() { - - } - - @Override - public void checkMessageDeduplicationInfo() { - - } - - @Override - public void incrementPublishCount(Producer producer, int numOfMessages, long msgSizeInBytes) { - - } - - @Override - public boolean shouldProducerMigrate() { - return false; - } - - @Override - public boolean isReplicationBacklogExist() { - return false; - } - - @Override - public CompletableFuture onPoliciesUpdate(Policies data) { - return null; - } - - @Override - public CompletableFuture checkBacklogQuotaExceeded(String producerName, - BacklogQuota.BacklogQuotaType backlogQuotaType) { - return null; - } - - @Override - public boolean isEncryptionRequired() { - return false; - } - - @Override - public boolean getSchemaValidationEnforced() { - return false; - } - - @Override - public boolean isReplicated() { - return false; - } - - @Override - public boolean isShadowReplicated() { - return false; - } - - @Override - public EntryFilters getEntryFiltersPolicy() { - return null; - } - - @Override - public List getEntryFilters() { - return List.of(); - } - - @Override - public BacklogQuota getBacklogQuota(BacklogQuota.BacklogQuotaType backlogQuotaType) { - return null; - } - - @Override - public long getBestEffortOldestUnacknowledgedMessageAgeSeconds() { - return 0; - } - - @Override - public void updateRates(NamespaceStats nsStats, NamespaceBundleStats currentBundleStats, - StatsOutputStream topicStatsStream, - ClusterReplicationMetrics clusterReplicationMetrics, - String namespaceName, boolean hydratePublishers) { - - } - - @Override - public Subscription getSubscription(String subscription) { - return null; - } - - @Override - public Map getReplicators() { - return Map.of(); - } - - @Override - public Map getShadowReplicators() { - return Map.of(); - } - - @Override - public TopicStatsImpl getStats(boolean getPreciseBacklog, boolean subscriptionBacklogSize, - boolean getEarliestTimeInBacklog) { - return null; - } - - @Override - public TopicStatsImpl getStats(GetStatsOptions getStatsOptions) { - return null; - } - - @Override - public CompletableFuture asyncGetStats(boolean getPreciseBacklog, - boolean subscriptionBacklogSize, - boolean getEarliestTimeInBacklog) { - return null; - } - - @Override - public CompletableFuture asyncGetStats(GetStatsOptions getStatsOptions) { - return null; - } - - @Override - public CompletableFuture getInternalStats(boolean includeLedgerMetadata) { - return null; - } - - @Override - public Position getLastPosition() { - return null; - } - - @Override - public CompletableFuture getLastMessageId() { - return null; - } - - @Override - public CompletableFuture hasSchema() { - return null; - } - - @Override - public CompletableFuture addSchema(SchemaData schema) { - return null; - } - - @Override - public CompletableFuture deleteSchema() { - return null; - } - - @Override - public CompletableFuture checkSchemaCompatibleForConsumer(SchemaData schema) { - return null; - } - - @Override - public CompletableFuture addSchemaIfIdleOrCheckCompatible(SchemaData schema) { - return null; - } - - @Override - public CompletableFuture deleteForcefully() { - return null; - } - - @Override - public boolean isPersistent() { - return false; - } - - @Override - public boolean isTransferring() { - return false; - } - - @Override - public void publishTxnMessage(TxnID txnID, ByteBuf headersAndPayload, PublishContext publishContext) { - - } - - @Override - public CompletableFuture endTxn(TxnID txnID, int txnAction, long lowWaterMark) { - return null; - } - - @Override - public CompletableFuture truncate() { - return null; - } - - @Override - public BrokerService getBrokerService() { - return MockBrokerService.create(); - } - - @Override - public HierarchyTopicPolicies getHierarchyTopicPolicies() { - return null; - } - - @Override - public TopicAttributes getTopicAttributes() { - return null; - } - } -} diff --git a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java index fb22ebcf17bb1..24e6094110236 100644 --- a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java +++ b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java @@ -27,7 +27,10 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import org.apache.bookkeeper.mledger.Position; -import org.apache.pulsar.broker.MockPersistentDispatcher; +import org.apache.bookkeeper.mledger.PositionFactory; +import org.apache.bookkeeper.mledger.impl.ActiveManagedCursorContainerImpl; +import org.apache.bookkeeper.mledger.impl.MockManagedCursor; +import org.apache.pulsar.broker.delayed.NoopDelayedDeliveryContext; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.BenchmarkMode; import org.openjdk.jmh.annotations.Fork; @@ -68,7 +71,7 @@ public class BucketDelayedDeliveryTrackerBenchmark { private BucketDelayedDeliveryTracker tracker; private Timer timer; private MockBucketSnapshotStorage storage; - private MockPersistentDispatcher dispatcher; + private NoopDelayedDeliveryContext context; private AtomicLong messageIdGenerator; @Setup(Level.Trial) @@ -92,12 +95,16 @@ public void tearDown() throws Exception { private void setupMockComponents() throws Exception { timer = new HashedWheelTimer(new DefaultThreadFactory("test-delayed-delivery"), 100, TimeUnit.MILLISECONDS); storage = new MockBucketSnapshotStorage(); - dispatcher = MockPersistentDispatcher.create(); + + ActiveManagedCursorContainerImpl container = new ActiveManagedCursorContainerImpl(); + MockManagedCursor cursor = MockManagedCursor.createCursor(container, "test-cursor", + PositionFactory.create(0, 0)); + context = new NoopDelayedDeliveryContext("delayed-JMH", cursor); } private void createTracker() throws Exception { tracker = new BucketDelayedDeliveryTracker( - dispatcher, timer, 1000, Clock.systemUTC(), true, storage, + context, timer, 1000, Clock.systemUTC(), true, storage, 20, 1000, 100, 50 ); } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/AbstractDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/AbstractDelayedDeliveryTracker.java index bec5134c4f79a..b873f6a411127 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/AbstractDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/AbstractDelayedDeliveryTracker.java @@ -29,7 +29,7 @@ @Slf4j public abstract class AbstractDelayedDeliveryTracker implements DelayedDeliveryTracker, TimerTask { - protected final AbstractPersistentDispatcherMultipleConsumers dispatcher; + protected final DelayedDeliveryContext context; // Reference to the shared (per-broker) timer for delayed delivery protected final Timer timer; @@ -49,23 +49,38 @@ public abstract class AbstractDelayedDeliveryTracker implements DelayedDeliveryT private final boolean isDelayedDeliveryDeliverAtTimeStrict; + private final Object timerStateLock = new Object(); + public AbstractDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumers dispatcher, Timer timer, long tickTimeMillis, boolean isDelayedDeliveryDeliverAtTimeStrict) { - this(dispatcher, timer, tickTimeMillis, Clock.systemUTC(), isDelayedDeliveryDeliverAtTimeStrict); + this(new DispatcherDelayedDeliveryContext(dispatcher), timer, tickTimeMillis, + Clock.systemUTC(), isDelayedDeliveryDeliverAtTimeStrict); } public AbstractDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumers dispatcher, Timer timer, long tickTimeMillis, Clock clock, boolean isDelayedDeliveryDeliverAtTimeStrict) { - this.dispatcher = dispatcher; + this(new DispatcherDelayedDeliveryContext(dispatcher), timer, tickTimeMillis, + clock, isDelayedDeliveryDeliverAtTimeStrict); + } + + public AbstractDelayedDeliveryTracker(DelayedDeliveryContext context, Timer timer, + long tickTimeMillis, + boolean isDelayedDeliveryDeliverAtTimeStrict) { + this(context, timer, tickTimeMillis, Clock.systemUTC(), isDelayedDeliveryDeliverAtTimeStrict); + } + + public AbstractDelayedDeliveryTracker(DelayedDeliveryContext context, Timer timer, + long tickTimeMillis, Clock clock, + boolean isDelayedDeliveryDeliverAtTimeStrict) { + this.context = context; this.timer = timer; this.tickTimeMillis = tickTimeMillis; this.clock = clock; this.isDelayedDeliveryDeliverAtTimeStrict = isDelayedDeliveryDeliverAtTimeStrict; } - /** * When {@link #isDelayedDeliveryDeliverAtTimeStrict} is false, we allow for early delivery by as much as the * {@link #tickTimeMillis} because it is a slight optimization to let messages skip going back into the delay @@ -89,72 +104,87 @@ public void resetTickTime(long tickTime) { protected void updateTimer() { if (getNumberOfDelayedMessages() == 0) { - if (timeout != null) { - currentTimeoutTarget = -1; - timeout.cancel(); - timeout = null; + synchronized (timerStateLock) { + if (timeout != null) { + currentTimeoutTarget = -1; + timeout.cancel(); + timeout = null; + } } return; } long timestamp = nextDeliveryTime(); - if (timestamp == currentTimeoutTarget) { - // The timer is already set to the correct target time - return; - } + synchronized (timerStateLock) { + if (timestamp == currentTimeoutTarget) { + // The timer is already set to the correct target time + return; + } - if (timeout != null) { - timeout.cancel(); - } + if (timeout != null) { + timeout.cancel(); + } - long now = clock.millis(); - long delayMillis = timestamp - now; + long now = clock.millis(); + long delayMillis = timestamp - now; + + if (delayMillis < 0) { + // There are messages that are already ready to be delivered. If + // the dispatcher is not getting them is because the consumer is + // either not connected or slow. + // We don't need to keep retriggering the timer. When the consumer + // catches up, the dispatcher will do the readMoreEntries() and + // get these messages + return; + } - if (delayMillis < 0) { - // There are messages that are already ready to be delivered. If - // the dispatcher is not getting them is because the consumer is - // either not connected or slow. - // We don't need to keep retriggering the timer. When the consumer - // catches up, the dispatcher will do the readMoreEntries() and - // get these messages - return; - } + // Compute the earliest time that we schedule the timer to run. + long remainingTickDelayMillis = lastTickRun + tickTimeMillis - now; + long calculatedDelayMillis = Math.max(delayMillis, remainingTickDelayMillis); - // Compute the earliest time that we schedule the timer to run. - long remainingTickDelayMillis = lastTickRun + tickTimeMillis - now; - long calculatedDelayMillis = Math.max(delayMillis, remainingTickDelayMillis); + if (log.isDebugEnabled()) { + log.debug("[{}] Start timer in {} millis", context.getName(), calculatedDelayMillis); + } - if (log.isDebugEnabled()) { - log.debug("[{}] Start timer in {} millis", dispatcher.getName(), calculatedDelayMillis); + // Even though we may delay longer than this timestamp because of the tick delay, we still track the + // current timeout with reference to the next message's timestamp. + currentTimeoutTarget = timestamp; + timeout = timer.newTimeout(this, calculatedDelayMillis, TimeUnit.MILLISECONDS); } + } - // Even though we may delay longer than this timestamp because of the tick delay, we still track the - // current timeout with reference to the next message's timestamp. - currentTimeoutTarget = timestamp; - timeout = timer.newTimeout(this, calculatedDelayMillis, TimeUnit.MILLISECONDS); + protected final void scheduleImmediateRun() { + synchronized (timerStateLock) { + if (timeout != null) { + timeout.cancel(); + } + timeout = timer.newTimeout(this, 0, TimeUnit.MILLISECONDS); + } } @Override public void run(Timeout timeout) throws Exception { if (log.isDebugEnabled()) { - log.debug("[{}] Timer triggered", dispatcher.getName()); + log.debug("[{}] Timer triggered", context.getName()); } if (timeout == null || timeout.isCancelled()) { return; } - synchronized (dispatcher) { + synchronized (timerStateLock) { lastTickRun = clock.millis(); currentTimeoutTarget = -1; this.timeout = null; - dispatcher.readMoreEntriesAsync(); } + context.triggerReadMoreEntries(); } @Override public void close() { - if (timeout != null) { - timeout.cancel(); - timeout = null; + synchronized (timerStateLock) { + if (timeout != null) { + timeout.cancel(); + timeout = null; + } } } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/BucketDelayedDeliveryTrackerFactory.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/BucketDelayedDeliveryTrackerFactory.java index 93eb3ebbc77d5..078a1f8ad38ea 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/BucketDelayedDeliveryTrackerFactory.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/BucketDelayedDeliveryTrackerFactory.java @@ -99,7 +99,18 @@ public DelayedDeliveryTracker newTracker(AbstractPersistentDispatcherMultipleCon @VisibleForTesting BucketDelayedDeliveryTracker newTracker0(AbstractPersistentDispatcherMultipleConsumers dispatcher) throws RecoverDelayedDeliveryTrackerException { - return new BucketDelayedDeliveryTracker(dispatcher, timer, tickTimeMillis, + DelayedDeliveryContext context = new DispatcherDelayedDeliveryContext(dispatcher); + return new BucketDelayedDeliveryTracker(context, timer, tickTimeMillis, + isDelayedDeliveryDeliverAtTimeStrict, bucketSnapshotStorage, delayedDeliveryMinIndexCountPerBucket, + TimeUnit.SECONDS.toMillis(delayedDeliveryMaxTimeStepPerBucketSnapshotSegmentSeconds), + delayedDeliveryMaxIndexesPerBucketSnapshotSegment, delayedDeliveryMaxNumBuckets); + } + + @VisibleForTesting + BucketDelayedDeliveryTracker newTracker0(String dispatcherName, ManagedCursor cursor) + throws RecoverDelayedDeliveryTrackerException { + DelayedDeliveryContext context = new NoopDelayedDeliveryContext(dispatcherName, cursor); + return new BucketDelayedDeliveryTracker(context, timer, tickTimeMillis, isDelayedDeliveryDeliverAtTimeStrict, bucketSnapshotStorage, delayedDeliveryMinIndexCountPerBucket, TimeUnit.SECONDS.toMillis(delayedDeliveryMaxTimeStepPerBucketSnapshotSegmentSeconds), delayedDeliveryMaxIndexesPerBucketSnapshotSegment, delayedDeliveryMaxNumBuckets); diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DelayedDeliveryContext.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DelayedDeliveryContext.java new file mode 100644 index 0000000000000..fe200e18c0337 --- /dev/null +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DelayedDeliveryContext.java @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker.delayed; + +import org.apache.bookkeeper.mledger.ManagedCursor; + +public interface DelayedDeliveryContext { + + String getName(); + + ManagedCursor getCursor(); + + void triggerReadMoreEntries(); +} diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DispatcherDelayedDeliveryContext.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DispatcherDelayedDeliveryContext.java new file mode 100644 index 0000000000000..91a8f4b234e77 --- /dev/null +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DispatcherDelayedDeliveryContext.java @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker.delayed; + +import org.apache.bookkeeper.mledger.ManagedCursor; +import org.apache.pulsar.broker.service.persistent.AbstractPersistentDispatcherMultipleConsumers; + +public class DispatcherDelayedDeliveryContext implements DelayedDeliveryContext { + + private final AbstractPersistentDispatcherMultipleConsumers dispatcher; + + public DispatcherDelayedDeliveryContext(AbstractPersistentDispatcherMultipleConsumers dispatcher) { + this.dispatcher = dispatcher; + } + + @Override + public String getName() { + return dispatcher.getName(); + } + + @Override + public ManagedCursor getCursor() { + return dispatcher.getCursor(); + } + + @Override + public void triggerReadMoreEntries() { + synchronized (dispatcher) { + dispatcher.readMoreEntriesAsync(); + } + } +} diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTracker.java index ad5ab25fbbf6b..93b1079390f21 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTracker.java @@ -33,6 +33,7 @@ import java.util.concurrent.atomic.AtomicLong; import lombok.Getter; import lombok.extern.slf4j.Slf4j; +import org.apache.bookkeeper.mledger.ManagedCursor; import org.apache.bookkeeper.mledger.Position; import org.apache.bookkeeper.mledger.PositionFactory; import org.apache.pulsar.broker.service.persistent.AbstractPersistentDispatcherMultipleConsumers; @@ -72,15 +73,39 @@ public class InMemoryDelayedDeliveryTracker extends AbstractDelayedDeliveryTrack long tickTimeMillis, boolean isDelayedDeliveryDeliverAtTimeStrict, long fixedDelayDetectionLookahead) { - this(dispatcher, timer, tickTimeMillis, Clock.systemUTC(), isDelayedDeliveryDeliverAtTimeStrict, - fixedDelayDetectionLookahead); + this(new DispatcherDelayedDeliveryContext(dispatcher), timer, tickTimeMillis, Clock.systemUTC(), + isDelayedDeliveryDeliverAtTimeStrict, fixedDelayDetectionLookahead); } public InMemoryDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumers dispatcher, Timer timer, long tickTimeMillis, Clock clock, boolean isDelayedDeliveryDeliverAtTimeStrict, long fixedDelayDetectionLookahead) { - super(dispatcher, timer, tickTimeMillis, clock, isDelayedDeliveryDeliverAtTimeStrict); + this(new DispatcherDelayedDeliveryContext(dispatcher), timer, tickTimeMillis, clock, + isDelayedDeliveryDeliverAtTimeStrict, fixedDelayDetectionLookahead); + } + + public InMemoryDelayedDeliveryTracker(String dispatcherName, ManagedCursor cursor, Timer timer, + long tickTimeMillis, Clock clock, + boolean isDelayedDeliveryDeliverAtTimeStrict, + long fixedDelayDetectionLookahead) { + this(new NoopDelayedDeliveryContext(dispatcherName, cursor), timer, tickTimeMillis, clock, + isDelayedDeliveryDeliverAtTimeStrict, fixedDelayDetectionLookahead); + } + + public InMemoryDelayedDeliveryTracker(DelayedDeliveryContext context, Timer timer, + long tickTimeMillis, + boolean isDelayedDeliveryDeliverAtTimeStrict, + long fixedDelayDetectionLookahead) { + this(context, timer, tickTimeMillis, Clock.systemUTC(), + isDelayedDeliveryDeliverAtTimeStrict, fixedDelayDetectionLookahead); + } + + public InMemoryDelayedDeliveryTracker(DelayedDeliveryContext context, Timer timer, + long tickTimeMillis, Clock clock, + boolean isDelayedDeliveryDeliverAtTimeStrict, + long fixedDelayDetectionLookahead) { + super(context, timer, tickTimeMillis, clock, isDelayedDeliveryDeliverAtTimeStrict); this.fixedDelayDetectionLookahead = fixedDelayDetectionLookahead; this.timestampPrecisionBitCnt = calculateTimestampPrecisionBitCnt(tickTimeMillis); } @@ -121,7 +146,7 @@ public boolean addMessage(long ledgerId, long entryId, long deliverAt) { } if (log.isDebugEnabled()) { - log.debug("[{}] Add message {}:{} -- Delivery in {} ms ", dispatcher.getName(), ledgerId, entryId, + log.debug("[{}] Add message {}:{} -- Delivery in {} ms ", context.getName(), ledgerId, entryId, deliverAt - clock.millis()); } @@ -213,7 +238,7 @@ public NavigableSet getScheduledMessages(int maxMessages) { } if (log.isDebugEnabled()) { - log.debug("[{}] Get scheduled messages - found {}", dispatcher.getName(), positions.size()); + log.debug("[{}] Get scheduled messages - found {}", context.getName(), positions.size()); } if (delayedMessageMap.isEmpty()) { @@ -222,7 +247,7 @@ public NavigableSet getScheduledMessages(int maxMessages) { messagesHaveFixedDelay = true; if (delayedMessagesCount.get() != 0) { log.warn("[{}] Delayed message tracker is empty, but delayedMessagesCount is {}", - dispatcher.getName(), delayedMessagesCount.get()); + context.getName(), delayedMessagesCount.get()); } } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTrackerFactory.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTrackerFactory.java index f8b8f5a8ba459..b0edfc455a527 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTrackerFactory.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTrackerFactory.java @@ -23,6 +23,7 @@ import io.netty.util.Timer; import io.netty.util.concurrent.DefaultThreadFactory; import java.util.concurrent.TimeUnit; +import org.apache.bookkeeper.mledger.ManagedCursor; import org.apache.pulsar.broker.PulsarService; import org.apache.pulsar.broker.ServiceConfiguration; import org.apache.pulsar.broker.service.persistent.AbstractPersistentDispatcherMultipleConsumers; @@ -67,7 +68,15 @@ public DelayedDeliveryTracker newTracker(AbstractPersistentDispatcherMultipleCon @VisibleForTesting InMemoryDelayedDeliveryTracker newTracker0(AbstractPersistentDispatcherMultipleConsumers dispatcher) { - return new InMemoryDelayedDeliveryTracker(dispatcher, timer, tickTimeMillis, + DelayedDeliveryContext context = new DispatcherDelayedDeliveryContext(dispatcher); + return new InMemoryDelayedDeliveryTracker(context, timer, tickTimeMillis, + isDelayedDeliveryDeliverAtTimeStrict, fixedDelayDetectionLookahead); + } + + @VisibleForTesting + InMemoryDelayedDeliveryTracker newTracker0(String dispatcherName, ManagedCursor cursor) { + DelayedDeliveryContext context = new NoopDelayedDeliveryContext(dispatcherName, cursor); + return new InMemoryDelayedDeliveryTracker(context, timer, tickTimeMillis, isDelayedDeliveryDeliverAtTimeStrict, fixedDelayDetectionLookahead); } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/NoopDelayedDeliveryContext.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/NoopDelayedDeliveryContext.java new file mode 100644 index 0000000000000..827fbd6cec553 --- /dev/null +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/NoopDelayedDeliveryContext.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pulsar.broker.delayed; + +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.bookkeeper.mledger.ManagedCursor; + +public class NoopDelayedDeliveryContext implements DelayedDeliveryContext { + + private final String name; + private final ManagedCursor cursor; + private final AtomicInteger triggerCount = new AtomicInteger(); + + public NoopDelayedDeliveryContext(String name, ManagedCursor cursor) { + this.name = name; + this.cursor = cursor; + } + + @Override + public String getName() { + return name; + } + + @Override + public ManagedCursor getCursor() { + return cursor; + } + + @Override + public void triggerReadMoreEntries() { + // no-op; for tests/JMH + triggerCount.incrementAndGet(); + } + + public int getTriggerCount() { + return triggerCount.get(); + } +} diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index e2adedae90069..2eeeba27ef568 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -60,6 +60,9 @@ import org.apache.commons.lang3.mutable.MutableLong; import org.apache.commons.lang3.tuple.Pair; import org.apache.pulsar.broker.delayed.AbstractDelayedDeliveryTracker; +import org.apache.pulsar.broker.delayed.DelayedDeliveryContext; +import org.apache.pulsar.broker.delayed.DispatcherDelayedDeliveryContext; +import org.apache.pulsar.broker.delayed.NoopDelayedDeliveryContext; import org.apache.pulsar.broker.delayed.proto.DelayedIndex; import org.apache.pulsar.broker.delayed.proto.SnapshotSegment; import org.apache.pulsar.broker.service.persistent.AbstractPersistentDispatcherMultipleConsumers; @@ -131,19 +134,55 @@ public BucketDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumer long minIndexCountPerBucket, long timeStepPerBucketSnapshotSegmentInMillis, int maxIndexesPerBucketSnapshotSegment, int maxNumBuckets) throws RecoverDelayedDeliveryTrackerException { - this(dispatcher, timer, tickTimeMillis, Clock.systemUTC(), isDelayedDeliveryDeliverAtTimeStrict, + this(new DispatcherDelayedDeliveryContext(dispatcher), timer, tickTimeMillis, Clock.systemUTC(), + isDelayedDeliveryDeliverAtTimeStrict, bucketSnapshotStorage, minIndexCountPerBucket, + timeStepPerBucketSnapshotSegmentInMillis, maxIndexesPerBucketSnapshotSegment, maxNumBuckets); + } + + public BucketDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumers dispatcher, + Timer timer, long tickTimeMillis, Clock clock, + boolean isDelayedDeliveryDeliverAtTimeStrict, + BucketSnapshotStorage bucketSnapshotStorage, + long minIndexCountPerBucket, long timeStepPerBucketSnapshotSegmentInMillis, + int maxIndexesPerBucketSnapshotSegment, int maxNumBuckets) + throws RecoverDelayedDeliveryTrackerException { + this(new DispatcherDelayedDeliveryContext(dispatcher), timer, tickTimeMillis, clock, + isDelayedDeliveryDeliverAtTimeStrict, bucketSnapshotStorage, minIndexCountPerBucket, + timeStepPerBucketSnapshotSegmentInMillis, maxIndexesPerBucketSnapshotSegment, maxNumBuckets); + } + + public BucketDelayedDeliveryTracker(String dispatcherName, ManagedCursor cursor, + Timer timer, long tickTimeMillis, + boolean isDelayedDeliveryDeliverAtTimeStrict, + BucketSnapshotStorage bucketSnapshotStorage, + long minIndexCountPerBucket, long timeStepPerBucketSnapshotSegmentInMillis, + int maxIndexesPerBucketSnapshotSegment, int maxNumBuckets) + throws RecoverDelayedDeliveryTrackerException { + this(new NoopDelayedDeliveryContext(dispatcherName, cursor), timer, tickTimeMillis, Clock.systemUTC(), + isDelayedDeliveryDeliverAtTimeStrict, bucketSnapshotStorage, minIndexCountPerBucket, + timeStepPerBucketSnapshotSegmentInMillis, maxIndexesPerBucketSnapshotSegment, maxNumBuckets); + } + + public BucketDelayedDeliveryTracker(DelayedDeliveryContext context, + Timer timer, long tickTimeMillis, + boolean isDelayedDeliveryDeliverAtTimeStrict, + BucketSnapshotStorage bucketSnapshotStorage, + long minIndexCountPerBucket, long timeStepPerBucketSnapshotSegmentInMillis, + int maxIndexesPerBucketSnapshotSegment, int maxNumBuckets) + throws RecoverDelayedDeliveryTrackerException { + this(context, timer, tickTimeMillis, Clock.systemUTC(), isDelayedDeliveryDeliverAtTimeStrict, bucketSnapshotStorage, minIndexCountPerBucket, timeStepPerBucketSnapshotSegmentInMillis, maxIndexesPerBucketSnapshotSegment, maxNumBuckets); } - public BucketDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumers dispatcher, + public BucketDelayedDeliveryTracker(DelayedDeliveryContext context, Timer timer, long tickTimeMillis, Clock clock, boolean isDelayedDeliveryDeliverAtTimeStrict, BucketSnapshotStorage bucketSnapshotStorage, long minIndexCountPerBucket, long timeStepPerBucketSnapshotSegmentInMillis, int maxIndexesPerBucketSnapshotSegment, int maxNumBuckets) throws RecoverDelayedDeliveryTrackerException { - super(dispatcher, timer, tickTimeMillis, clock, isDelayedDeliveryDeliverAtTimeStrict); + super(context, timer, tickTimeMillis, clock, isDelayedDeliveryDeliverAtTimeStrict); this.minIndexCountPerBucket = minIndexCountPerBucket; this.timeStepPerBucketSnapshotSegmentInMillis = timeStepPerBucketSnapshotSegmentInMillis; this.maxIndexesPerBucketSnapshotSegment = maxIndexesPerBucketSnapshotSegment; @@ -152,7 +191,7 @@ public BucketDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumer this.immutableBuckets = TreeRangeMap.create(); this.snapshotSegmentLastIndexMap = new ConcurrentHashMap<>(); this.lastMutableBucket = - new MutableBucket(dispatcher.getName(), dispatcher.getCursor(), FutureUtil.Sequencer.create(), + new MutableBucket(context.getName(), context.getCursor(), FutureUtil.Sequencer.create(), bucketSnapshotStorage); this.stats = new BucketDelayedMessageIndexStats(); ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock(); @@ -161,7 +200,6 @@ public BucketDelayedDeliveryTracker(AbstractPersistentDispatcherMultipleConsumer bucketSnapshotExecutor = Executors.newSingleThreadScheduledExecutor( new ExecutorProvider.ExtendedThreadFactory("bucket-creation")); - // Close the tracker if failed to recover. try { long recoveredMessages = recoverBucketSnapshot(); this.numberDelayedMessages.set(recoveredMessages); @@ -176,7 +214,7 @@ private synchronized long recoverBucketSnapshot() throws RecoverDelayedDeliveryT Map cursorProperties = cursor.getCursorProperties(); if (MapUtils.isEmpty(cursorProperties)) { log.info("[{}] Recover delayed message index bucket snapshot finish, don't find bucket snapshot", - dispatcher.getName()); + context.getName()); return 0; } FutureUtil.Sequencer sequencer = this.lastMutableBucket.getSequencer(); @@ -186,7 +224,7 @@ private synchronized long recoverBucketSnapshot() throws RecoverDelayedDeliveryT String[] keys = key.split(DELIMITER); checkArgument(keys.length == 3); ImmutableBucket immutableBucket = - new ImmutableBucket(dispatcher.getName(), cursor, sequencer, + new ImmutableBucket(context.getName(), cursor, sequencer, this.lastMutableBucket.bucketSnapshotStorage, Long.parseLong(keys[1]), Long.parseLong(keys[2])); putAndCleanOverlapRange(Range.closed(immutableBucket.startLedgerId, immutableBucket.endLedgerId), @@ -197,7 +235,7 @@ private synchronized long recoverBucketSnapshot() throws RecoverDelayedDeliveryT Map, ImmutableBucket> immutableBucketMap = immutableBuckets.asMapOfRanges(); if (immutableBucketMap.isEmpty()) { log.info("[{}] Recover delayed message index bucket snapshot finish, don't find bucket snapshot", - dispatcher.getName()); + context.getName()); return 0; } @@ -211,7 +249,7 @@ private synchronized long recoverBucketSnapshot() throws RecoverDelayedDeliveryT try { FutureUtil.waitForAll(futures.values()).get(AsyncOperationTimeoutSeconds * 5, TimeUnit.SECONDS); } catch (InterruptedException | ExecutionException | TimeoutException e) { - log.error("[{}] Failed to recover delayed message index bucket snapshot.", dispatcher.getName(), e); + log.error("[{}] Failed to recover delayed message index bucket snapshot.", context.getName(), e); if (e instanceof InterruptedException) { Thread.currentThread().interrupt(); } @@ -252,7 +290,7 @@ private synchronized long recoverBucketSnapshot() throws RecoverDelayedDeliveryT }); log.info("[{}] Recover delayed message index bucket snapshot finish, buckets: {}, numberDelayedMessages: {}", - dispatcher.getName(), immutableBucketMap.size(), numberDelayedMessages.getValue()); + context.getName(), immutableBucketMap.size(), numberDelayedMessages.getValue()); return numberDelayedMessages.getValue(); } @@ -343,7 +381,7 @@ private void afterCreateImmutableBucket(Pair immu if (ex == null) { immutableBucket.setSnapshotSegments(null); immutableBucket.asyncUpdateSnapshotLength(); - log.info("[{}] Create bucket snapshot finish, bucketKey: {}", dispatcher.getName(), + log.info("[{}] Create bucket snapshot finish, bucketKey: {}", context.getName(), immutableBucket.bucketKey()); stats.recordSuccessEvent(BucketDelayedMessageIndexStats.Type.create, @@ -352,7 +390,7 @@ private void afterCreateImmutableBucket(Pair immu return bucketId; } - log.error("[{}] Failed to create bucket snapshot, bucketKey: {}", dispatcher.getName(), + log.error("[{}] Failed to create bucket snapshot, bucketKey: {}", context.getName(), immutableBucket.bucketKey(), ex); stats.recordFailEvent(BucketDelayedMessageIndexStats.Type.create); @@ -426,7 +464,7 @@ public boolean addMessage(long ledgerId, long entryId, long deliverAt) { && bucketSnapshotInProgress.compareAndSet(false, true)) { // Create bucket snapshot this.bucketBeingSealed = this.lastMutableBucket; - this.lastMutableBucket = new MutableBucket(dispatcher.getName(), dispatcher.getCursor(), + this.lastMutableBucket = new MutableBucket(context.getName(), context.getCursor(), FutureUtil.Sequencer.create(), this.lastMutableBucket.getBucketSnapshotStorage()); bucketSnapshotExecutor.execute(() -> { try { @@ -452,7 +490,7 @@ public boolean addMessage(long ledgerId, long entryId, long deliverAt) { numberDelayedMessages.incrementAndGet(); if (log.isDebugEnabled()) { - log.debug("[{}] Add message {}:{} -- Delivery in {} ms ", dispatcher.getName(), ledgerId, entryId, + log.debug("[{}] Add message {}:{} -- Delivery in {} ms ", context.getName(), ledgerId, entryId, deliverAt - clock.millis()); } @@ -507,14 +545,14 @@ private CompletableFuture asyncMergeBucketSnapshot() { List toBeMergeImmutableBuckets = selectMergedBuckets(immutableBucketList, MAX_MERGE_NUM); if (toBeMergeImmutableBuckets.isEmpty()) { - log.warn("[{}] Can't find able merged buckets", dispatcher.getName()); + log.warn("[{}] Can't find able merged buckets", context.getName()); return CompletableFuture.completedFuture(null); } final String bucketsStr = toBeMergeImmutableBuckets.stream().map(Bucket::bucketKey).collect( Collectors.joining(",")).replaceAll(DELAYED_BUCKET_KEY_PREFIX + "_", ""); if (log.isDebugEnabled()) { - log.info("[{}] Merging bucket snapshot, bucketKeys: {}", dispatcher.getName(), bucketsStr); + log.info("[{}] Merging bucket snapshot, bucketKeys: {}", context.getName(), bucketsStr); } for (ImmutableBucket immutableBucket : toBeMergeImmutableBuckets) { @@ -534,12 +572,12 @@ private CompletableFuture asyncMergeBucketSnapshot() { } if (ex != null) { log.error("[{}] Failed to merge bucket snapshot, bucketKeys: {}", - dispatcher.getName(), bucketsStr, ex); + context.getName(), bucketsStr, ex); stats.recordFailEvent(BucketDelayedMessageIndexStats.Type.merge); } else { log.info("[{}] Merge bucket snapshot finish, bucketKeys: {}, bucketNum: {}", - dispatcher.getName(), bucketsStr, immutableBuckets.asMapOfRanges().size()); + context.getName(), bucketsStr, immutableBuckets.asMapOfRanges().size()); stats.recordSuccessEvent(BucketDelayedMessageIndexStats.Type.merge, System.currentTimeMillis() - mergeStartTime); @@ -712,7 +750,7 @@ public NavigableSet getScheduledMessages(int maxMessages) { if (pendingLoad != null && !pendingLoad.isDone()) { if (log.isDebugEnabled()) { log.debug("[{}] Skip getScheduledMessages to wait for bucket snapshot load finish.", - dispatcher.getName()); + context.getName()); } return Collections.emptyNavigableSet(); } @@ -760,13 +798,13 @@ private void triggerAsyncLoadBucketSnapshot(ImmutableBucket bucketToLoad, Snapsh final int preSegmentEntryId = bucketToLoad.currentSegmentEntryId; if (log.isDebugEnabled()) { log.debug("[{}] Loading next bucket snapshot segment, bucketKey: {}, nextSegmentEntryId: {}", - dispatcher.getName(), bucketToLoad.bucketKey(), preSegmentEntryId + 1); + context.getName(), bucketToLoad.bucketKey(), preSegmentEntryId + 1); } boolean createFutureDone = bucketToLoad.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE).isDone(); if (!createFutureDone) { log.info("[{}] Skip load to wait for bucket snapshot create finish, bucketKey:{}", - dispatcher.getName(), bucketToLoad.bucketKey()); + context.getName(), bucketToLoad.bucketKey()); return; } @@ -801,26 +839,18 @@ private void triggerAsyncLoadBucketSnapshot(ImmutableBucket bucketToLoad, Snapsh bucketToLoad.setCurrentSegmentEntryId(preSegmentEntryId); log.error("[{}] Failed to load bucket snapshot segment, bucketKey: {}, segmentEntryId: {}", - dispatcher.getName(), bucketToLoad.bucketKey(), preSegmentEntryId + 1, ex); + context.getName(), bucketToLoad.bucketKey(), preSegmentEntryId + 1, ex); stats.recordFailEvent(BucketDelayedMessageIndexStats.Type.load); } else { log.info("[{}] Load next bucket snapshot segment finish, bucketKey: {}, segmentEntryId: {}", - dispatcher.getName(), bucketToLoad.bucketKey(), + context.getName(), bucketToLoad.bucketKey(), (preSegmentEntryId == bucketToLoad.lastSegmentEntryId) ? "-1" : preSegmentEntryId + 1); stats.recordSuccessEvent(BucketDelayedMessageIndexStats.Type.load, System.currentTimeMillis() - loadStartTime); } - writeLock.lock(); - try { - if (timeout != null) { - timeout.cancel(); - } - timeout = timer.newTimeout(this, 0, TimeUnit.MILLISECONDS); - } finally { - writeLock.unlock(); - } + scheduleImmediateRun(); }); } @@ -856,7 +886,7 @@ public void close() { completableFutures = immutableBuckets.asMapOfRanges().values().stream() .map(bucket -> bucket.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE)).toList(); } catch (Exception e) { - log.warn("[{}] Failed wait to snapshot generate", dispatcher.getName(), e); + log.warn("[{}] Failed wait to snapshot generate", context.getName(), e); } } finally { writeLock.unlock(); @@ -866,7 +896,7 @@ public void close() { FutureUtil.waitForAll(completableFutures).get(AsyncOperationTimeoutSeconds, TimeUnit.SECONDS); } } catch (Exception e) { - log.warn("[{}] Failed wait to snapshot generate", dispatcher.getName(), e); + log.warn("[{}] Failed wait to snapshot generate", context.getName(), e); } finally { bucketSnapshotExecutor.shutdown(); } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerTest.java index 05f8bb1505369..fe2d60bbf49b5 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerTest.java @@ -313,7 +313,7 @@ public void testMergeSnapshot(final BucketDelayedDeliveryTracker tracker) throws clockTime.set(110 * 10); NavigableSet scheduledMessages = new TreeSet<>(); - Awaitility.await().untilAsserted(() -> { + Awaitility.await().atMost(20, TimeUnit.SECONDS).untilAsserted(() -> { scheduledMessages.addAll(tracker2.getScheduledMessages(110)); assertEquals(scheduledMessages.size(), 110); }); From 738d0e2c41cdf7cc727789f348ebeda4089b9353 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Sun, 30 Nov 2025 12:21:09 +0800 Subject: [PATCH 13/16] feat(delayed): enhance bucket sealing and message routing logic for improved thread safety --- .../apache/pulsar/broker/package-info.java | 2 +- .../bucket/BucketDelayedDeliveryTracker.java | 187 +++++++++++++----- ...elayedDeliveryTrackerThreadSafetyTest.java | 6 +- 3 files changed, 143 insertions(+), 52 deletions(-) diff --git a/microbench/src/main/java/org/apache/pulsar/broker/package-info.java b/microbench/src/main/java/org/apache/pulsar/broker/package-info.java index bb5c91c6d31ac..a7620134f2f84 100644 --- a/microbench/src/main/java/org/apache/pulsar/broker/package-info.java +++ b/microbench/src/main/java/org/apache/pulsar/broker/package-info.java @@ -22,6 +22,6 @@ * *

This package contains JMH benchmarks for testing the performance * characteristics of the BucketDelayedDeliveryTracker, particularly - * focusing on thread safety improvements with StampedLock optimistic reads. + * focusing on thread safety improvements with ReentrantReadWriteLock. */ package org.apache.pulsar.broker; \ No newline at end of file diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index 2eeeba27ef568..19179c359b56f 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -122,6 +122,24 @@ public static record SnapshotKey(long ledgerId, long entryId) {} private final ExecutorService bucketSnapshotExecutor; private final AtomicBoolean bucketSnapshotInProgress = new AtomicBoolean(false); + /** + * Bucket that is currently being sealed into an immutable bucket. + * + *

Lifecycle: + *

    + *
  • When a mutable bucket reaches the threshold to be persisted, it is assigned to + * {@code bucketBeingSealed} under the {@link #writeLock} and a fresh {@link #lastMutableBucket} + * is created for new messages.
    • + *
  • While a bucket is being sealed, messages whose ledger id falls into its range are not written + * to this bucket anymore. Instead they are routed directly to {@link #sharedBucketPriorityQueue} + * and de-duplication is done using the current {@link #lastMutableBucket} bitmap.
    • + *
  • Once {@link #createBucketSnapshotAsync(MutableBucket)} finishes (successfully or exceptionally), + * this field is cleared. This transition also happens under the {@link #writeLock}.
    • + *
+ * + *

All access to this field must be done under {@link #writeLock} to keep bucket routing and sealing + * consistent across threads.

+ */ private volatile MutableBucket bucketBeingSealed = null; private final Lock readLock; @@ -198,7 +216,7 @@ public BucketDelayedDeliveryTracker(DelayedDeliveryContext context, this.readLock = rwLock.readLock(); this.writeLock = rwLock.writeLock(); bucketSnapshotExecutor = Executors.newSingleThreadScheduledExecutor( - new ExecutorProvider.ExtendedThreadFactory("bucket-creation")); + new ExecutorProvider.ExtendedThreadFactory(context.getName() + "bucket-creation")); try { long recoveredMessages = recoverBucketSnapshot(); @@ -422,6 +440,30 @@ private void afterCreateImmutableBucket(Pair immu } } + /** + * Add a delayed message to the tracker. + * + *

Routing rules (under {@link #writeLock} for the mutating part): + *

    + *
  • If the message already exists, it is ignored.
    • + *
  • If the delivery time is before the cutoff, it is dropped.
    • + *
  • If the current {@link #lastMutableBucket} has accumulated enough indexes and the message + * ledger id is strictly after its range, a snapshot is triggered: + *
      + *
    • The current {@code lastMutableBucket} is moved to {@link #bucketBeingSealed}.
      • + *
    • A fresh {@code lastMutableBucket} is created for subsequent messages.
      • + *
    • Sealing and persistence happen asynchronously in + * {@link #createBucketSnapshotAsync(MutableBucket)}.
      • + *
    + *
    • + *
  • Messages whose ledger id falls into the range of a bucket that is currently being sealed, or + * into an already immutable bucket, or before the current {@code lastMutableBucket} range, + * are routed directly to {@link #sharedBucketPriorityQueue} and tracked only via the bitmap + * of the current {@code lastMutableBucket}. This mirrors the original behaviour where + * messages for closed ranges are not re-added to mutable buckets.
    • + *
  • All remaining messages are added to {@code lastMutableBucket}.
    • + *
+ */ @Override public boolean addMessage(long ledgerId, long entryId, long deliverAt) { readLock.lock(); @@ -449,42 +491,41 @@ public boolean addMessage(long ledgerId, long entryId, long deliverAt) { } final MutableBucket sealingBucket = this.bucketBeingSealed; - if (sealingBucket != null + final boolean sealingBucketContainsLedger = sealingBucket != null && ledgerId >= sealingBucket.startLedgerId - && ledgerId <= sealingBucket.endLedgerId) { + && ledgerId <= sealingBucket.endLedgerId; + + boolean existBucket = findImmutableBucket(ledgerId).isPresent(); + + if (!sealingBucketContainsLedger + && !existBucket + && ledgerId > lastMutableBucket.endLedgerId + && lastMutableBucket.size() >= minIndexCountPerBucket + && !lastMutableBucket.isEmpty() + && bucketSnapshotInProgress.compareAndSet(false, true)) { + // Create bucket snapshot using current lastMutableBucket as the bucket to seal. + final MutableBucket bucketToSeal = this.lastMutableBucket; + this.bucketBeingSealed = bucketToSeal; + this.lastMutableBucket = new MutableBucket(context.getName(), context.getCursor(), + FutureUtil.Sequencer.create(), bucketToSeal.getBucketSnapshotStorage()); + bucketSnapshotExecutor.execute(() -> { + try { + createBucketSnapshotAsync(bucketToSeal); + } finally { + bucketSnapshotInProgress.set(false); + } + }); + } + + if (sealingBucketContainsLedger || ledgerId < lastMutableBucket.startLedgerId || existBucket) { + // If message belongs to a bucket currently being sealed, or an existing immutable bucket, + // or has ledgerId smaller than current lastMutableBucket range, we put it directly into + // the shared queue and track its index bit in the current mutable bucket. sharedBucketPriorityQueue.add(deliverAt, ledgerId, entryId); - sealingBucket.putIndexBit(ledgerId, entryId); + lastMutableBucket.putIndexBit(ledgerId, entryId); } else { - boolean existBucket = findImmutableBucket(ledgerId).isPresent(); - - if (!existBucket - && ledgerId > lastMutableBucket.endLedgerId - && lastMutableBucket.size() >= minIndexCountPerBucket - && !lastMutableBucket.isEmpty() - && bucketSnapshotInProgress.compareAndSet(false, true)) { - // Create bucket snapshot - this.bucketBeingSealed = this.lastMutableBucket; - this.lastMutableBucket = new MutableBucket(context.getName(), context.getCursor(), - FutureUtil.Sequencer.create(), this.lastMutableBucket.getBucketSnapshotStorage()); - bucketSnapshotExecutor.execute(() -> { - try { - createBucketSnapshotAsync(); - } finally { - bucketSnapshotInProgress.set(false); - } - }); - } - - if (ledgerId < lastMutableBucket.startLedgerId || existBucket) { - // If (ledgerId < startLedgerId || existBucket) - // means that message index belong to previous bucket range, - // enter sharedBucketPriorityQueue directly - sharedBucketPriorityQueue.add(deliverAt, ledgerId, entryId); - lastMutableBucket.putIndexBit(ledgerId, entryId); - } else { - checkArgument(ledgerId >= lastMutableBucket.endLedgerId); - lastMutableBucket.addMessage(ledgerId, entryId, deliverAt); - } + checkArgument(ledgerId >= lastMutableBucket.endLedgerId); + lastMutableBucket.addMessage(ledgerId, entryId, deliverAt); } numberDelayedMessages.incrementAndGet(); @@ -660,13 +701,32 @@ private CompletableFuture asyncMergeBucketSnapshot(List b }); } - private void createBucketSnapshotAsync() { - final MutableBucket bucketToSeal = this.bucketBeingSealed; - if (bucketToSeal == null || bucketToSeal.isEmpty()) { - this.bucketBeingSealed = null; + /** + * Seal the given mutable bucket into an immutable one and persist its snapshot. + * + *

This method is always executed on {@link #bucketSnapshotExecutor} and never mutates the + * actively written {@link #lastMutableBucket}. The {@code bucketToSeal} instance is detached + * from writes before this method is scheduled.

+ * + *

On success, {@link #afterCreateImmutableBucket(Pair, long)} is called under the + * {@link #writeLock}, the new immutable bucket is registered in {@link #immutableBuckets}, + * and {@link #bucketBeingSealed} is cleared. On failure or empty bucket, only the + * {@code bucketBeingSealed} state is cleared.

+ * + * @param bucketToSeal the mutable bucket that was selected under the write lock when the + * snapshot was triggered; it must not be modified by callers afterwards + */ + private void createBucketSnapshotAsync(MutableBucket bucketToSeal) { + if (bucketToSeal == null) { return; } + try { + if (bucketToSeal.isEmpty()) { + clearBucketBeingSealed(bucketToSeal); + return; + } + long createStartTime = System.currentTimeMillis(); stats.recordTriggerEvent(BucketDelayedMessageIndexStats.Type.create); Pair immutableBucketDelayedIndexPair = @@ -675,15 +735,14 @@ private void createBucketSnapshotAsync() { this.maxIndexesPerBucketSnapshotSegment, this.sharedBucketPriorityQueue); if (immutableBucketDelayedIndexPair == null) { + clearBucketBeingSealed(bucketToSeal); return; } writeLock.lock(); try { afterCreateImmutableBucket(immutableBucketDelayedIndexPair, createStartTime); - if (this.bucketBeingSealed == bucketToSeal) { - this.bucketBeingSealed = null; - } + clearBucketBeingSealedUnsafe(bucketToSeal); if (maxNumBuckets > 0 && immutableBuckets.asMapOfRanges().size() > maxNumBuckets) { asyncMergeBucketSnapshot(); @@ -691,16 +750,42 @@ private void createBucketSnapshotAsync() { } finally { writeLock.unlock(); } + } catch (Throwable t) { + log.error("[{}] Failed to create bucket snapshot", context.getName(), t); + clearBucketBeingSealed(bucketToSeal); + } + } + + /** + * Clear {@link #bucketBeingSealed} if it still refers to the given bucket. + * + *

This method acquires the {@link #writeLock}. Use + * {@link #clearBucketBeingSealedUnsafe(MutableBucket)} instead when the caller already + * holds the write lock.

+ */ + private void clearBucketBeingSealed(MutableBucket bucketToSeal) { + writeLock.lock(); + try { + clearBucketBeingSealedUnsafe(bucketToSeal); } finally { - if (this.bucketBeingSealed == bucketToSeal) { - this.bucketBeingSealed = null; - } + writeLock.unlock(); + } + } + + /** + * Clear {@link #bucketBeingSealed} without acquiring the {@link #writeLock}. + * + *

Callers must already hold {@link #writeLock} when invoking this method.

+ */ + private void clearBucketBeingSealedUnsafe(MutableBucket bucketToSeal) { + if (this.bucketBeingSealed == bucketToSeal) { + this.bucketBeingSealed = null; } } @Override public boolean hasMessageAvailable() { - writeLock.lock(); + readLock.lock(); try { long cutoffTime = getCutoffTime(); @@ -710,7 +795,7 @@ public boolean hasMessageAvailable() { } return hasMessageAvailable; } finally { - writeLock.unlock(); + readLock.unlock(); } } @@ -899,6 +984,16 @@ public void close() { log.warn("[{}] Failed wait to snapshot generate", context.getName(), e); } finally { bucketSnapshotExecutor.shutdown(); + try { + if (!bucketSnapshotExecutor.awaitTermination(AsyncOperationTimeoutSeconds, TimeUnit.SECONDS)) { + log.warn("[{}] bucketSnapshotExecutor did not terminate in the specified time.", + context.getName()); + } + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + log.warn("[{}] Interrupted while waiting for bucketSnapshotExecutor to terminate.", + context.getName(), ie); + } } } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java index 31362b68436dd..20b76939a4b7c 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerThreadSafetyTest.java @@ -55,7 +55,6 @@ public class BucketDelayedDeliveryTrackerThreadSafetyTest { private BucketDelayedDeliveryTracker tracker; private AbstractPersistentDispatcherMultipleConsumers dispatcher; - private ManagedCursor cursor; private Timer timer; private BucketSnapshotStorage storage; private ExecutorService executorService; @@ -63,7 +62,7 @@ public class BucketDelayedDeliveryTrackerThreadSafetyTest { @BeforeMethod public void setUp() throws Exception { dispatcher = mock(AbstractPersistentDispatcherMultipleConsumers.class); - cursor = mock(ManagedCursor.class); + final ManagedCursor cursor = mock(ManagedCursor.class); timer = mock(Timer.class); storage = mock(BucketSnapshotStorage.class); @@ -1003,9 +1002,6 @@ public void testMixedReadWriteOperationsDeadlockDetection() throws Exception { } } - // Awaitility.await().atMost(30, TimeUnit.SECONDS).untilAsserted(() -> - // assertEquals(deliverableMessagesCount.get(), totalMessages)); - // Verify operations completed successfully assertTrue(completedOperations.get() > 0, "Some operations should complete"); assertTrue(messagesAdded.get() > 0, "Some messages should have been added"); From 5a73d69cb8971f4b782ac1487e5a8c834389d099 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Wed, 3 Dec 2025 22:01:41 +0800 Subject: [PATCH 14/16] feat(delayed): improve topic name extraction and ensure sequential message addition in benchmarks --- ...BucketDelayedDeliveryTrackerBenchmark.java | 38 +++++++++++++++---- .../pulsar/broker/delayed/bucket/Bucket.java | 18 ++++++++- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java index 24e6094110236..701e42ab9665e 100644 --- a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java +++ b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java @@ -30,6 +30,7 @@ import org.apache.bookkeeper.mledger.PositionFactory; import org.apache.bookkeeper.mledger.impl.ActiveManagedCursorContainerImpl; import org.apache.bookkeeper.mledger.impl.MockManagedCursor; +import org.apache.pulsar.broker.delayed.DelayedDeliveryTracker; import org.apache.pulsar.broker.delayed.NoopDelayedDeliveryContext; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.BenchmarkMode; @@ -73,6 +74,16 @@ public class BucketDelayedDeliveryTrackerBenchmark { private MockBucketSnapshotStorage storage; private NoopDelayedDeliveryContext context; private AtomicLong messageIdGenerator; + /** + * In real Pulsar usage, {@link DelayedDeliveryTracker#addMessage(long, long, long)} is invoked + * by a single dispatcher thread and messages arrive in order of (ledgerId, entryId). + *

+ * To reflect this invariant in the benchmark, all write operations that end up calling + * {@code tracker.addMessage(...)} are serialized via this mutex so that the tracker only + * ever observes a single writer with monotonically increasing ids, even when JMH runs the + * benchmark method with multiple threads. + */ + private final Object writeMutex = new Object(); @Setup(Level.Trial) public void setup() throws Exception { @@ -99,7 +110,10 @@ private void setupMockComponents() throws Exception { ActiveManagedCursorContainerImpl container = new ActiveManagedCursorContainerImpl(); MockManagedCursor cursor = MockManagedCursor.createCursor(container, "test-cursor", PositionFactory.create(0, 0)); - context = new NoopDelayedDeliveryContext("delayed-JMH", cursor); + // Use the same " / " naming pattern as real dispatchers, + // so that Bucket.asyncSaveBucketSnapshot can correctly derive topicName. + String dispatcherName = "persistent://public/default/jmh-topic / " + cursor.getName(); + context = new NoopDelayedDeliveryContext(dispatcherName, cursor); } private void createTracker() throws Exception { @@ -135,6 +149,19 @@ public boolean benchmarkMixedOperations() { } } + /** + * Serialize calls to {@link BucketDelayedDeliveryTracker#addMessage(long, long, long)} and + * ensure (ledgerId, entryId) are generated in a strictly increasing sequence, matching the + * real dispatcher single-threaded behaviour. + */ + private boolean addMessageSequential(long deliverAt, int entryIdModulo) { + synchronized (writeMutex) { + long id = messageIdGenerator.getAndIncrement(); + long entryId = id % entryIdModulo; + return tracker.addMessage(id, entryId, deliverAt); + } + } + private boolean performReadOperation() { int operation = ThreadLocalRandom.current().nextInt(3); switch (operation) { @@ -161,9 +188,8 @@ private boolean performReadOperation() { } private boolean performWriteOperation() { - long id = messageIdGenerator.getAndIncrement(); long deliverAt = System.currentTimeMillis() + ThreadLocalRandom.current().nextLong(5000, 30000); - return tracker.addMessage(id, id % 1000, deliverAt); + return addMessageSequential(deliverAt, 1000); } // ============================================================================= @@ -181,9 +207,8 @@ public boolean benchmarkConcurrentContainsMessage() { @Benchmark @Threads(4) public boolean benchmarkConcurrentAddMessage() { - long id = messageIdGenerator.getAndIncrement(); long deliverAt = System.currentTimeMillis() + ThreadLocalRandom.current().nextLong(10000, 60000); - return tracker.addMessage(id, id % 1000, deliverAt); + return addMessageSequential(deliverAt, 1000); } @Benchmark @@ -192,8 +217,7 @@ public NavigableSet benchmarkConcurrentGetScheduledMessages() { // Create some messages ready for delivery long currentTime = System.currentTimeMillis(); for (int i = 0; i < 5; i++) { - long id = messageIdGenerator.getAndIncrement(); - tracker.addMessage(id, id % 100, currentTime - 1000); + addMessageSequential(currentTime - 1000, 100); } return tracker.getScheduledMessages(10); } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/Bucket.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/Bucket.java index a1693b1553d97..c355405846312 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/Bucket.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/Bucket.java @@ -137,7 +137,23 @@ CompletableFuture asyncSaveBucketSnapshot( List bucketSnapshotSegments) { final String bucketKey = bucket.bucketKey(); final String cursorName = Codec.decode(cursor.getName()); - final String topicName = dispatcherName.substring(0, dispatcherName.lastIndexOf(" / " + cursorName)); + final String suffix = " / " + cursorName; + final int suffixIndex = dispatcherName.lastIndexOf(suffix); + final String topicName; + if (suffixIndex >= 0) { + topicName = dispatcherName.substring(0, suffixIndex); + } else { + // Fallback for dispatcher names that don't follow the " / " pattern. + // This can happen in tests or benchmarks that use a simplified dispatcher name. + // Using the full dispatcherName as topicName avoids StringIndexOutOfBoundsException + // while still providing a meaningful identifier to the snapshot storage. + topicName = dispatcherName; + if (log.isDebugEnabled()) { + log.debug("Dispatcher name '{}' does not contain expected suffix '{}', " + + "using full dispatcherName as topic name", + dispatcherName, suffix); + } + } return executeWithRetry( () -> bucketSnapshotStorage.createBucketSnapshot(snapshotMetadata, bucketSnapshotSegments, bucketKey, topicName, cursorName) From cc1a635157066616ac8c4b41485e1697d94a7ad3 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Thu, 4 Dec 2025 20:32:15 +0800 Subject: [PATCH 15/16] feat(delayed): optimize bucket merging logic and enhance benchmark parameters for memory management --- ...BucketDelayedDeliveryTrackerBenchmark.java | 35 ++++++++++++++++--- .../bucket/BucketDelayedDeliveryTracker.java | 10 ++++++ .../CombinedSegmentDelayedIndexQueue.java | 18 +++++++++- 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java index 701e42ab9665e..f0faa1f6d3ab6 100644 --- a/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java +++ b/microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java @@ -58,15 +58,15 @@ @BenchmarkMode(Mode.Throughput) @OutputTimeUnit(TimeUnit.SECONDS) @State(Scope.Benchmark) -@Warmup(iterations = 3, time = 2) -@Measurement(iterations = 5, time = 3) +@Warmup(time = 10, timeUnit = TimeUnit.SECONDS, iterations = 1) +@Measurement(time = 10, timeUnit = TimeUnit.SECONDS, iterations = 1) @Fork(1) public class BucketDelayedDeliveryTrackerBenchmark { @Param({"90_10", "80_20", "70_30", "50_50"}) public String readWriteRatio; - @Param({"1000", "5000", "10000"}) + @Param({"1000", "5000", "8000"}) public int initialMessages; private BucketDelayedDeliveryTracker tracker; @@ -74,6 +74,22 @@ public class BucketDelayedDeliveryTrackerBenchmark { private MockBucketSnapshotStorage storage; private NoopDelayedDeliveryContext context; private AtomicLong messageIdGenerator; + /** + * Maximum number of additional unique (ledgerId, entryId) positions to + * introduce per trial on top of {@link #initialMessages}. This allows + * controlling the memory footprint of the benchmark while still applying + * sustained write pressure to the tracker. + * + *

Use {@code -p maxAdditionalUniqueMessages=...} on the JMH command line + * to tune the load. The default value is conservative for local runs.

+ */ + @Param({"1000000"}) + public long maxAdditionalUniqueMessages; + /** + * Upper bound on the absolute message id that will be used to derive + * (ledgerId, entryId) positions during a single trial. + */ + private long maxUniqueMessageId; /** * In real Pulsar usage, {@link DelayedDeliveryTracker#addMessage(long, long, long)} is invoked * by a single dispatcher thread and messages arrive in order of (ledgerId, entryId). @@ -91,6 +107,9 @@ public void setup() throws Exception { createTracker(); preloadMessages(); messageIdGenerator = new AtomicLong(initialMessages + 1); + // Allow a bounded number of additional unique messages per trial to avoid + // unbounded memory growth while still stressing the indexing logic. + maxUniqueMessageId = initialMessages + maxAdditionalUniqueMessages; } @TearDown(Level.Trial) @@ -157,8 +176,14 @@ public boolean benchmarkMixedOperations() { private boolean addMessageSequential(long deliverAt, int entryIdModulo) { synchronized (writeMutex) { long id = messageIdGenerator.getAndIncrement(); - long entryId = id % entryIdModulo; - return tracker.addMessage(id, entryId, deliverAt); + // Limit the number of distinct positions that are introduced into the tracker + // to keep memory usage bounded. Once the upper bound is reached, we re-use + // the last position id so that subsequent calls behave like updates to + // existing messages and are short-circuited by containsMessage checks. + long boundedId = Math.min(id, maxUniqueMessageId); + long ledgerId = boundedId; + long entryId = boundedId % entryIdModulo; + return tracker.addMessage(ledgerId, entryId, deliverAt); } } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index 19179c359b56f..3abe60bc74de1 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -660,6 +660,16 @@ private CompletableFuture asyncMergeBucketSnapshot(List b buckets.get(0).startLedgerId, buckets.get(buckets.size() - 1).endLedgerId); + if (immutableBucketDelayedIndexPair == null) { + // No delayed indexes remained in the segments to merge. + // Keep the existing buckets as-is and skip creating a new one. + log.warn("[{}] Skip merging bucket snapshot, no remaining indexes found, " + + "startLedgerId: {}, endLedgerId: {}", + context.getName(), buckets.get(0).startLedgerId, + buckets.get(buckets.size() - 1).endLedgerId); + return; + } + // Merge bit map to new bucket Map delayedIndexBitMap = new HashMap<>(buckets.get(0).getDelayedIndexBitMap()); diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/CombinedSegmentDelayedIndexQueue.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/CombinedSegmentDelayedIndexQueue.java index 006938e9ed271..526b7d7532729 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/CombinedSegmentDelayedIndexQueue.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/CombinedSegmentDelayedIndexQueue.java @@ -48,7 +48,23 @@ static class Node { private CombinedSegmentDelayedIndexQueue(List> segmentLists) { this.kpq = new PriorityQueue<>(segmentLists.size(), COMPARATOR_NODE); for (List segmentList : segmentLists) { - Node node = new Node(segmentList, 0, 0); + if (segmentList == null || segmentList.isEmpty()) { + // Skip empty segment lists, there is nothing to merge from them. + continue; + } + + // Advance to the first non-empty segment in this list. + int segmentListCursor = 0; + while (segmentListCursor < segmentList.size() + && segmentList.get(segmentListCursor).getIndexesCount() == 0) { + segmentListCursor++; + } + if (segmentListCursor >= segmentList.size()) { + // All segments are empty, skip this list entirely. + continue; + } + + Node node = new Node(segmentList, segmentListCursor, 0); kpq.offer(node); } } From 43d7fafe73bdddef08caa8bb0209b06e6c19e951 Mon Sep 17 00:00:00 2001 From: Denovo1998 Date: Sun, 7 Dec 2025 21:26:44 +0800 Subject: [PATCH 16/16] feat(delayed): streamline bucket snapshot handling and improve executor shutdown process --- .../bucket/BucketDelayedDeliveryTracker.java | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java index 3abe60bc74de1..df5fe87f907d0 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java @@ -663,10 +663,6 @@ private CompletableFuture asyncMergeBucketSnapshot(List b if (immutableBucketDelayedIndexPair == null) { // No delayed indexes remained in the segments to merge. // Keep the existing buckets as-is and skip creating a new one. - log.warn("[{}] Skip merging bucket snapshot, no remaining indexes found, " - + "startLedgerId: {}, endLedgerId: {}", - context.getName(), buckets.get(0).startLedgerId, - buckets.get(buckets.size() - 1).endLedgerId); return; } @@ -867,6 +863,13 @@ public NavigableSet getScheduledMessages(int maxMessages) { SnapshotKey snapshotKey = new SnapshotKey(ledgerId, entryId); ImmutableBucket bucket = snapshotSegmentLastIndexMap.get(snapshotKey); if (bucket != null && immutableBuckets.asMapOfRanges().containsValue(bucket)) { + // All message of current snapshot segment are scheduled, try load next snapshot segment + if (bucket.merging) { + log.info("[{}] Skip load to wait for bucket snapshot merge finish, bucketKey:{}", + context.getName(), bucket.bucketKey()); + break; + } + // This is the last message of a segment. We need to load the next one. // Trigger the load and stop processing more messages in this run. // The positions collected so far will be returned. @@ -971,12 +974,29 @@ public CompletableFuture clear() { @Override public void close() { - List> completableFutures = Collections.emptyList(); writeLock.lock(); try { super.close(); lastMutableBucket.close(); sharedBucketPriorityQueue.close(); + } finally { + writeLock.unlock(); + } + bucketSnapshotExecutor.shutdown(); + try { + if (!bucketSnapshotExecutor.awaitTermination(AsyncOperationTimeoutSeconds, TimeUnit.SECONDS)) { + log.warn("[{}] bucketSnapshotExecutor did not terminate in the specified time.", + context.getName()); + } + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + log.warn("[{}] Interrupted while waiting for bucketSnapshotExecutor to terminate.", + context.getName(), ie); + } + + List> completableFutures = Collections.emptyList(); + writeLock.lock(); + try { try { completableFutures = immutableBuckets.asMapOfRanges().values().stream() .map(bucket -> bucket.getSnapshotCreateFuture().orElse(NULL_LONG_PROMISE)).toList(); @@ -986,24 +1006,13 @@ public void close() { } finally { writeLock.unlock(); } + try { if (!completableFutures.isEmpty()) { FutureUtil.waitForAll(completableFutures).get(AsyncOperationTimeoutSeconds, TimeUnit.SECONDS); } } catch (Exception e) { log.warn("[{}] Failed wait to snapshot generate", context.getName(), e); - } finally { - bucketSnapshotExecutor.shutdown(); - try { - if (!bucketSnapshotExecutor.awaitTermination(AsyncOperationTimeoutSeconds, TimeUnit.SECONDS)) { - log.warn("[{}] bucketSnapshotExecutor did not terminate in the specified time.", - context.getName()); - } - } catch (InterruptedException ie) { - Thread.currentThread().interrupt(); - log.warn("[{}] Interrupted while waiting for bucketSnapshotExecutor to terminate.", - context.getName(), ie); - } } } @@ -1046,7 +1055,6 @@ private boolean containsMessageUnsafe(long ledgerId, long entryId) { .orElse(false); } - public Map genTopicMetricMap() { stats.recordNumOfBuckets(immutableBuckets.asMapOfRanges().size() + 1); stats.recordDelayedMessageIndexLoaded(this.sharedBucketPriorityQueue.size() + this.lastMutableBucket.size());