Skip to content

[improve][broker] Optimize fine-grained concurrency control for BucketDelayedDeliveryTracker - #24739

Draft
Denovo1998 wants to merge 28 commits into
apache:masterfrom
Denovo1998:bucket_delivery_tracker_optimize
Draft

[improve][broker] Optimize fine-grained concurrency control for BucketDelayedDeliveryTracker#24739
Denovo1998 wants to merge 28 commits into
apache:masterfrom
Denovo1998:bucket_delivery_tracker_optimize

Conversation

@Denovo1998

@Denovo1998 Denovo1998 commented Sep 14, 2025

Copy link
Copy Markdown
Contributor

Fixes #24603

Main Issue: #24600

PIP: #xyz

Motivation

  • Reduce lock contention and fix thread-safety issues in BucketDelayedDeliveryTracker.
  • Remove blocking I/O from the publish path.
  • Decouple the tracker from dispatcher for easier testing and benchmarking.

Modifications

  • Concurrency
    • Replace StampedLock and synchronized with ReentrantReadWriteLock (read/write separation).
    • addMessage uses “read-lock check → write-lock modify” to minimize write lock time.
  • Asynchronous snapshots
    • Seal-and-swap the current bucket and persist snapshots on a single-thread executor.
    • Ensure only one snapshot is created at a time.
  • Scheduling and timer
    • getScheduledMessages triggers async loading of next snapshot segment instead of blocking.
    • Reworked timer handling with a dedicated lock and scheduleImmediateRun; no synchronization on dispatcher.
  • Decoupling
    • Introduce DelayedDeliveryContext:
      • DispatcherDelayedDeliveryContext (production).
      • NoopDelayedDeliveryContext (tests/benchmarks).
    • Trackers and factories accept dispatcher or context; added constructors for name+cursor.
  • Merging and lifecycle
    • asyncMergeBucketSnapshot uses write lock; merges bitmaps; removes old buckets; improved stats/logging.
    • clear() and close() under write lock; wait for snapshot futures outside locks; shutdown snapshot executor.
  • Tests and benchmarks
    • Expanded unit and thread-safety tests; made MockManagedCursor public.
    • New JMH suite (BucketDelayedDeliveryTrackerBenchmark) and MockBucketSnapshotStorage; removed old simple benchmark.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: Denovo1998#10

…rategy

Refactor lock mechanism from StampedLock to ReentrantReadWriteLock for thread safety. Add async bucket snapshot creation and improve concurrent message handling.
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Sep 14, 2025
@Denovo1998

Copy link
Copy Markdown
Contributor Author

@codelipenghui @lhotari @coderzc @Apurva007
How does this copy on write method look, is my direction correct?
There is still much work to be done, and I will convert this PR to a draft later.

@lhotari

lhotari commented Sep 14, 2025

Copy link
Copy Markdown
Member

@codelipenghui @lhotari @coderzc @Apurva007
How does this copy on write method look, is my direction correct?
There is still much work to be done, and I will convert this PR to a draft later.

Without benchmarking, it will be hard to validate assumptions. I'd suggest adding JMH benchmarks so that performance could be compared.

JMH benchmarks can be added to microbench module: https://github.com/apache/pulsar/tree/master/microbench

@Denovo1998

Copy link
Copy Markdown
Contributor Author

OK. I will add JMH benchmarks first.

@Denovo1998
Denovo1998 marked this pull request as draft September 17, 2025 12:40
Comment thread microbench/src/main/java/org/apache/pulsar/broker/MockPersistentDispatcher.java Outdated
@Denovo1998
Denovo1998 marked this pull request as ready for review September 21, 2025 01:37
# Conflicts:
#	microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerBenchmark.java
#	microbench/src/main/java/org/apache/pulsar/broker/delayed/bucket/MockBucketSnapshotStorage.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/AbstractDelayedDeliveryTracker.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DelayedDeliveryContext.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DispatcherDelayedDeliveryContext.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTracker.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTrackerFactory.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/NoopDelayedDeliveryContext.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java
# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/AbstractDelayedDeliveryTracker.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/InMemoryDelayedDeliveryTracker.java
#	pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/InMemoryDeliveryTrackerTest.java
@lhotari

lhotari commented Aug 28, 2026

Copy link
Copy Markdown
Member

Is this still relevant?

@Denovo1998

Copy link
Copy Markdown
Contributor Author

@lhotari Recently, there have been many changes about delaying messages. I will start to re-study this pr next week.

Merge the current upstream master while intentionally taking its delayed-delivery implementation as the new baseline. The original PR implementation predates the shared runtime index, bucket context, asynchronous close, trim safeguards, and related correctness fixes; the focused snapshot-build offload is rebuilt in follow-up commits.
…cker lock

Detach and replace the active mutable bucket under the tracker monitor, then build and persist its snapshot outside the critical section.

Use a broker-scoped bounded executor with memory-mode fallback, and fence asynchronous work across clear and close lifecycle transitions.

Cover rejection, build and persistence failures, stale lifecycle completions, late messages, and non-blocking close with deterministic tests.
Measure the ledger-rollover add latency for 10K, 100K, and 1M-index buckets with direct and asynchronous snapshot builders.

Use Gradle benchmark packaging and longer JMH warmup, measurement, and fork settings for repeatable comparisons.
@Denovo1998
Denovo1998 marked this pull request as draft September 2, 2026 10:53
@Denovo1998

Copy link
Copy Markdown
Contributor Author

@lhotari
I have modified a version locally, and I think I may need to wait for these two pr to merge before continuing.

#26401

#26420

Let‘s finish the review and merge of these two pr first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs release/4.1.4 release/4.2.5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] The Bucket-based delayed queue has serious performance issues

6 participants