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
[improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true #17465
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.
great finding !
Nice optimization |
public int cardinality(long lowerKey, long lowerValue, long upperKey, long upperValue) { | ||
NavigableMap<Long, BitSet> subMap = rangeBitSetMap.subMap(lowerKey, true, upperKey, true); | ||
MutableInt v = new MutableInt(0); | ||
subMap.forEach((ledgerId, bitset) -> { |
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.
Since the class ConcurrentOpenLongPairRangeSet
isn't specific to ledgers, would it make sense to avoid using ledgerId
as the variable name?
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/LongPairRangeSet.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenLongPairRangeSet.java
Outdated
Show resolved
Hide resolved
@lhotari Thanks for the review. I have updated the PR according to your suggestions; please help take a look again. |
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 +1
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
…sOpenCacheSetEnabled=true (#17465)
@codelipenghui LGTM, great improvement in this PR. |
…sOpenCacheSetEnabled=true (apache#17465) (cherry picked from commit 09edcce) (cherry picked from commit a88ac65)
Fixes #17451
Motivation
Improve the
ManagedCursorImpl.getNumberOfEntries()
ifisUnackedRangesOpenCacheSetEnabled=true
.Since the
ConcurrentOpenLongPairRangeSet
has the ability to get the acked count for each Ledger,the
ManagedCursorImpl
can avoid iterating each range to calculate the acked count.Modifications
Make the
ConcurrentOpenLongPairRangeSet
able to get acked count.Verifying this change
Unit test for the new method introduced in
ConcurrentOpenLongPairRangeSet
The
getNumberOfEntries()
change is already covered by the existing testDocumentation
Check the box below or label this PR directly.
Need to update docs?
doc-not-needed
(Just an improvement for the presented implementation)