You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "Apurva007 (via GitHub)" <gi...@apache.org> on 2025/07/21 21:21:42 UTC
[PR] [fix][broker]Fix thread safety issues in BucketDelayedDeliveryTracker with StampedLock optimistic reads [pulsar]
Apurva007 opened a new pull request, #24542:
URL: https://github.com/apache/pulsar/pull/24542
### Motivation
Fixes #23190
BucketDelayedDeliveryTracker had thread safety issues in frequently called methods like `containsMessage()` and `nextDeliveryTime()` that could lead to race conditions, incorrect duplicate detection, and scheduling inconsistencies under high concurrency loads.
The issue manifested as:
- Race conditions in `containsMessage()` leading to incorrect duplicate detection
- Concurrent access issues in `nextDeliveryTime()` causing scheduling inconsistencies
- Potential data corruption under high concurrency scenarios
### Modifications
- **Added StampedLock for high-performance concurrency control**
- Implemented optimistic read pattern for frequently called read operations
- Provides lock-free fast path when no concurrent writes are occurring
- Falls back gracefully to read locks when validation fails
- **Applied optimistic reads to critical methods:**
- `containsMessage()` - Used for duplicate message detection
- `nextDeliveryTime()` - Called frequently for message scheduling
- **Maintained existing write operation synchronization**
- Write operations continue to use `synchronized` for simplicity and safety
- Mixed approach optimal for typical read-heavy delayed delivery workloads
- **Removed unused data structure**
- Eliminated unused `immutableBucketsMap` field to reduce memory overhead
- All bucket operations use the existing `immutableBuckets` RangeMap
### Performance Improvements
Benchmark results show excellent performance across various concurrency scenarios:
- **Single-threaded reads**: ~305 million ops/s
- **High concurrency (16 threads)**: ~2.6 billion ops/s
- **Mixed read/write ratios**: Consistent performance from 10:90 to 90:10
- **Optimistic read success rate**: Very high under typical read-heavy workloads
### Thread Safety Strategy
- **Read operations**: Use StampedLock optimistic reads for maximum performance
- **Write operations**: Continue using synchronized for safety and simplicity
- **Data structures**: Leverage existing thread-safe collections (ConcurrentHashMap, etc.)
### Verifying this change
- **Added comprehensive thread safety test**: `BucketDelayedDeliveryTrackerThreadSafetyTest`
- **Created performance benchmark**: `BucketDelayedDeliveryTrackerSimpleBenchmark`
- **All existing tests pass**
- **No functional changes** - maintains full backward compatibility
### 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 behavior
- [ ] The cluster topology
- [ ] The ARM (kafka compatibility, producer/consumer compatibility)
### Documentation
- [ ] doc-required (This change needs documentation updates)
- [x] doc-not-needed (This change does not need documentation updates)
- [ ] doc-complete (This change includes documentation updates)
### Matching PR in forked repository
PR in forked repository: N/A
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <no...@anthropic.com>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker]Fix thread safety issues in BucketDelayedDeliveryTracker with StampedLock optimistic reads [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24542:
URL: https://github.com/apache/pulsar/pull/24542#issuecomment-3133064281
@Apurva007 The test added in this PR found a concurrency bug that currently reproduces at least in branch-3.0/branch-3.3: https://github.com/apache/pulsar/issues/24578 .
One detail that I noticed about this PR is that the StampedLock's `writeLock` is never used, making it useless. I'm sorry I missed that in the review. It seems to be harder to make BucketDelayedDeliveryTracker fully thread safe. One challenge in using StampedLock's write locks is that the lock isn't re-entrant and would lead to a deadlock if the same thread tries to acquire the write lock a second time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker]Fix thread safety issues in BucketDelayedDeliveryTracker with StampedLock optimistic reads [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui merged PR #24542:
URL: https://github.com/apache/pulsar/pull/24542
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker]Fix thread safety issues in BucketDelayedDeliveryTracker with StampedLock optimistic reads [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24542:
URL: https://github.com/apache/pulsar/pull/24542#issuecomment-3133120918
Reopened #23190
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker]Fix thread safety issues in BucketDelayedDeliveryTracker with StampedLock optimistic reads [pulsar]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24542:
URL: https://github.com/apache/pulsar/pull/24542#issuecomment-3099098745
@Apurva007 Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker]Fix thread safety issues in BucketDelayedDeliveryTracker with StampedLock optimistic reads [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24542:
URL: https://github.com/apache/pulsar/pull/24542#issuecomment-3099760260
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/24542?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `78.37838%` with `8 lines` in your changes missing coverage. Please review.
> Project coverage is 74.26%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`975ff1f`)](https://app.codecov.io/gh/apache/pulsar/commit/975ff1f23d579ffc49fbc12a86def464254de4d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1208 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24542?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...r/delayed/bucket/BucketDelayedDeliveryTracker.java](https://app.codecov.io/gh/apache/pulsar/pull/24542?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fdelayed%2Fbucket%2FBucketDelayedDeliveryTracker.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9CdWNrZXREZWxheWVkRGVsaXZlcnlUcmFja2VyLmphdmE=) | 78.37% | [6 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/24542?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/24542?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #24542 +/- ##
============================================
+ Coverage 73.57% 74.26% +0.69%
- Complexity 32624 32898 +274
============================================
Files 1877 1869 -8
Lines 139502 146113 +6611
Branches 15299 16763 +1464
============================================
+ Hits 102638 108518 +5880
- Misses 28908 28964 +56
- Partials 7956 8631 +675
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/24542/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/24542/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.74% <0.00%> (+2.15%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/24542/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.31% <0.00%> (-1.01%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/24542/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.77% <78.37%> (+0.92%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/24542?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...r/delayed/bucket/BucketDelayedDeliveryTracker.java](https://app.codecov.io/gh/apache/pulsar/pull/24542?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fdelayed%2Fbucket%2FBucketDelayedDeliveryTracker.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9CdWNrZXREZWxheWVkRGVsaXZlcnlUcmFja2VyLmphdmE=) | `81.83% <78.37%> (-1.87%)` | :arrow_down: |
... and [1101 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/24542/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org