You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/05 04:24:51 UTC
[GitHub] [pulsar] codelipenghui opened a new pull request, #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
codelipenghui opened a new pull request, #17465:
URL: https://github.com/apache/pulsar/pull/17465
Fixes #17451
### Motivation
Improve the `ManagedCursorImpl.getNumberOfEntries()` if `isUnackedRangesOpenCacheSetEnabled=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 test
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [x] `doc-not-needed`
(Just an improvement for the presented implementation)
--
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
[GitHub] [pulsar] lhotari commented on a diff in pull request #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17465:
URL: https://github.com/apache/pulsar/pull/17465#discussion_r962884348
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenLongPairRangeSet.java:
##########
@@ -242,6 +242,27 @@ public Range<T> lastRange() {
return Range.openClosed(consumer.apply(lastSet.getKey(), lower), consumer.apply(lastSet.getKey(), upper));
}
+ @Override
+ 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) -> {
+ if (ledgerId == lowerKey || ledgerId == upperKey) {
+ BitSet temp = (BitSet) bitset.clone();
+ if (ledgerId == lowerKey) {
+ temp.clear(0, (int) Math.max(0, lowerValue));
+ }
+ if (ledgerId == upperKey) {
+ temp.clear((int) Math.min(upperValue + 1, temp.length()), temp.length());
+ }
Review Comment:
I seem to understand why it's needed, however it would be useful to add a comment
--
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
[GitHub] [pulsar] lhotari commented on pull request #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #17465:
URL: https://github.com/apache/pulsar/pull/17465#issuecomment-1236974781
Nice optimization
--
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
[GitHub] [pulsar] codelipenghui commented on pull request #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #17465:
URL: https://github.com/apache/pulsar/pull/17465#issuecomment-1237634039
@lhotari Thanks for the review. I have updated the PR according to your suggestions; please help take a look again.
--
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
[GitHub] [pulsar] lhotari commented on a diff in pull request #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17465:
URL: https://github.com/apache/pulsar/pull/17465#discussion_r962877620
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenLongPairRangeSet.java:
##########
@@ -242,6 +242,27 @@ public Range<T> lastRange() {
return Range.openClosed(consumer.apply(lastSet.getKey(), lower), consumer.apply(lastSet.getKey(), upper));
}
+ @Override
+ 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) -> {
Review Comment:
Since the class `ConcurrentOpenLongPairRangeSet` isn't specific to ledgers, would it make sense to avoid using `ledgerId` as the variable name?
--
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
[GitHub] [pulsar] lhotari commented on a diff in pull request #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17465:
URL: https://github.com/apache/pulsar/pull/17465#discussion_r962885293
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/LongPairRangeSet.java:
##########
@@ -125,6 +125,11 @@
*/
Range<T> lastRange();
+ /**
+ * Return the number bit sets to true from lower to upper.
Review Comment:
for example "Return the number bit sets to true from lower (inclusive) to upper (inclusive)."
--
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
[GitHub] [pulsar] lhotari commented on a diff in pull request #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17465:
URL: https://github.com/apache/pulsar/pull/17465#discussion_r962883066
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentOpenLongPairRangeSet.java:
##########
@@ -242,6 +242,27 @@ public Range<T> lastRange() {
return Range.openClosed(consumer.apply(lastSet.getKey(), lower), consumer.apply(lastSet.getKey(), upper));
}
+ @Override
+ 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) -> {
+ if (ledgerId == lowerKey || ledgerId == upperKey) {
+ BitSet temp = (BitSet) bitset.clone();
+ if (ledgerId == lowerKey) {
+ temp.clear(0, (int) Math.max(0, lowerValue));
+ }
+ if (ledgerId == upperKey) {
+ temp.clear((int) Math.min(upperValue + 1, temp.length()), temp.length());
+ }
Review Comment:
why is this logic needed?
--
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
[GitHub] [pulsar] lhotari commented on pull request #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #17465:
URL: https://github.com/apache/pulsar/pull/17465#issuecomment-1241551821
> @lhotari Thanks for the review. I have updated the PR according to your suggestions; please help take a look again.
@codelipenghui LGTM, great improvement in this PR.
--
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
[GitHub] [pulsar] Technoboy- merged pull request #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #17465:
URL: https://github.com/apache/pulsar/pull/17465
--
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
[GitHub] [pulsar] lhotari commented on a diff in pull request #17465: [improve][broker] Improve cursor.getNumberOfEntries if isUnackedRangesOpenCacheSetEnabled=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17465:
URL: https://github.com/apache/pulsar/pull/17465#discussion_r962881704
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/LongPairRangeSet.java:
##########
@@ -125,6 +125,11 @@
*/
Range<T> lastRange();
+ /**
+ * Return the number bit sets to true from lower to upper.
Review Comment:
please add a comment about the boundary inclusiveness.
--
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