-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix][broker]Wrong backlog: expected 0 but got 1 #24938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an incorrect backlog calculation bug that occurs when resetting a subscription with negative entry IDs. The root cause was in the getNumberOfEntries method which didn't properly handle positions with negative entry IDs (e.g., -1), causing it to add a negative value to the count instead of treating it as 0.
- Fixed
getNumberOfEntries()to handle negativetoPositionentry IDs - Added
comparePositions()helper method to properly compare positions that may be invalid - Added test to reproduce and verify the fix
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
ManagedLedgerImpl.java |
Fixed getNumberOfEntries() to check if toPosition.getEntryId() < 0 before adding it to count |
ManagedCursorImpl.java |
Added comparePositions() helper method and updated internalResetCursor() to use it for proper position comparison |
SimpleProducerConsumerTest.java |
Added test case with two scenarios (with/without ledger trimming) to verify backlog calculation after subscription creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #24938 +/- ##
============================================
- Coverage 74.34% 74.34% -0.01%
- Complexity 33496 33700 +204
============================================
Files 1913 1920 +7
Lines 149315 150282 +967
Branches 17331 17444 +113
============================================
+ Hits 111012 111725 +713
- Misses 29475 29663 +188
- Partials 8828 8894 +66
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
(cherry picked from commit ed31d82)
(cherry picked from commit ed31d82)
Motivation
The backlog was calculated by
managedLedger.entriesAddedCounter - cursor.messagesConsumedCounterWhen resetting a subscription, the broker resets the value
cursor.messagesConsumedCounterthis way:{new value} = {old value} - {how many positions the old value is larger than the new value}.The method
managedLegder.getNumberOfEntriesis used to calculate the number between two positions, but it has a bug, which will get a larger value than expected if the entry ID is a negative value.You can reproduce the issue by the new test
[1] : https://github.com/apache/pulsar/blob/v4.0.7/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L3730C5-L3764C6
Modifications
position.comparateDocumentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: x