-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix][broker] fix delay queue sequence issue. #24035
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
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.
LGTM
@thetumbled Is it #23611 where the regression was introduced? Does the previous implementation have a similar issue? |
The previous implementation has sequence issue too, but triggered by other way. Previouse implementation sort the triple tuple (timestamp, ledgerid, entryid) with heap sort algorithm, which is not a stable sort method.
These three messages are scheduled to be delivered at the same time, but the dispatch sequence may not be 0, 1, 2 due to the sort algorithm. |
(cherry picked from commit 998bb51)
This reverts commit addae4b.
This reverts commit addae4b.
Motivation
When a group of delay messages reach to theire timestamp, we expect that the dispatch sequence in accordance with the message id.
Yet there is corner case that will break this rule, we can reproduce the problem with the unit test
testDelaySequence
in this pr.The root reason is that the inner map of
delayedMessageMap
isLong2ObjectMap<Roaring64Bitmap>
instead ofLong2ObjectSortedMap<Roaring64Bitmap>
.Modifications
Change the type of
delayedMessageMap
toLong2ObjectSortedMap<Long2ObjectSortedMap<Roaring64Bitmap>>
.Verifying this change
(Please pick either of the following options)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: thetumbled#72