You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "gaoran10 (via GitHub)" <gi...@apache.org> on 2025/03/05 07:30:27 UTC
[PR] [fix] Avoid negative estimated entry count [pulsar]
gaoran10 opened a new pull request, #24055:
URL: https://github.com/apache/pulsar/pull/24055
### Motivation
The estimated entry count may be a negative value in some cases.
### Modifications
Add a negative check after the long value converts to an int value.
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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] Avoid negative estimated entry count [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1981725310
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3855,7 +3854,8 @@ static long estimateEntryCountBySize(long bytesSize, Position readPosition, Mana
posToRead = ml.getNextValidPosition(PositionFactory.create(posToRead.getLedgerId(), Long.MAX_VALUE));
}
}
- return Math.max(result, 1);
+ int resultInt = Long.valueOf(result).intValue();
+ return Math.max(resultInt < 0 ? Integer.MAX_VALUE : resultInt, 1);
Review Comment:
Another note is that `Long.valueOf(result).intValue()` should be avoided since it creates an unnecessary `java.lang.Long` instance.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#issuecomment-2703532267
Add the same `release/xxx` labels since it's fixing the regression of https://github.com/apache/pulsar/pull/23931
--
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] Avoid negative estimated entry count [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#issuecomment-2701410663
> The estimated entry count may be a negative value in some cases, then the read operation will get an empty result.
@gaoran10 Just wondering how it could result could exceed `Integer.MAX_VALUE` so that the integer value becomes negative in the current logic. Is it the line `result += remainEntriesOfLedger;` in `estimateEntryCountBySize` method that causes it? Could we also improve the existing logic and add some test cases to reproduce and prevent such regressions?
--
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] Avoid negative estimated entry count [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1981719682
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3855,7 +3854,8 @@ static long estimateEntryCountBySize(long bytesSize, Position readPosition, Mana
posToRead = ml.getNextValidPosition(PositionFactory.create(posToRead.getLedgerId(), Long.MAX_VALUE));
}
}
- return Math.max(result, 1);
+ int resultInt = Long.valueOf(result).intValue();
+ return Math.max(resultInt < 0 ? Integer.MAX_VALUE : resultInt, 1);
Review Comment:
A better solution would be to check the long value before converting it, since in theory, it could "wrap around" multiple times.
```suggestion
return Math.max(result > Integer.MAX_VALUE ? (long) Integer.MAX_VALUE : result, 1L);
```
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982611559
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3855,7 +3854,8 @@ static long estimateEntryCountBySize(long bytesSize, Position readPosition, Mana
posToRead = ml.getNextValidPosition(PositionFactory.create(posToRead.getLedgerId(), Long.MAX_VALUE));
}
}
- return Math.max(result, 1);
+ int resultInt = Long.valueOf(result).intValue();
+ return Math.max(resultInt < 0 ? Integer.MAX_VALUE : resultInt, 1);
Review Comment:
I changed the method return type from long to int, because we still need to convert it from long to int when using it.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982621617
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3855,7 +3854,8 @@ static long estimateEntryCountBySize(long bytesSize, Position readPosition, Mana
posToRead = ml.getNextValidPosition(PositionFactory.create(posToRead.getLedgerId(), Long.MAX_VALUE));
}
}
- return Math.max(result, 1);
+ int resultInt = Long.valueOf(result).intValue();
+ return Math.max(resultInt < 0 ? Integer.MAX_VALUE : resultInt, 1);
Review Comment:
> Another note is that Long.valueOf(result).intValue() should be avoided since it creates an unnecessary java.lang.Long instance.
fixed, thanks.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982654770
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,12 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ return Math.min(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger), maxEntries);
Review Comment:
I just checked the downstream side, which could call `asyncReadEntries(maxEntries, Long.MAX_VALUE, ...)` directly to only limit by `maxEntries`.
A more proper fix should be:
```java
final var estimateEntryCount = estimateEntryCountBySize(maxSizeBytes, readPosition, ledger);
if (estimateEntryCount >= Integer.MAX_VALUE) {
return maxEntries;
}
return Math.min((int) estimateEntryCount, maxEntries);
```
And we should add tests to `applyMaxSizeCap` rather than the internal `estimateEntryCountBySize` method.
This PR changes the semantic of `estimateEntryCountBySize` that when the average size is greater than `Integer.MAX_VALUE`, it will return `Integer.MAX_VALUE`, which is anti-intuitive.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982654770
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,12 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ return Math.min(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger), maxEntries);
Review Comment:
I just checked the downstream side, which could call `asyncReadEntries(maxEntries, Long.MAX_VALUE, ...)` to only limit by `maxEntries`.
A more proper fix should be:
```java
final var estimateEntryCount = estimateEntryCountBySize(maxSizeBytes, readPosition, ledger);
if (estimateEntryCount >= Integer.MAX_VALUE) {
return maxEntries;
}
return Math.min((int) estimateEntryCount, maxEntries);
```
And we should add tests to `applyMaxSizeCap` rather than the internal `estimateEntryCountBySize` method.
This PR changes the semantic of `estimateEntryCountBySize` that when the average size is greater than `Integer.MAX_VALUE`, it will return `Integer.MAX_VALUE`, which is anti-intuitive.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#issuecomment-2729365543
There was an additional regression caused by #23931 changes. I've created a PR #24089 to address it. Please review
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982621617
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3855,7 +3854,8 @@ static long estimateEntryCountBySize(long bytesSize, Position readPosition, Mana
posToRead = ml.getNextValidPosition(PositionFactory.create(posToRead.getLedgerId(), Long.MAX_VALUE));
}
}
- return Math.max(result, 1);
+ int resultInt = Long.valueOf(result).intValue();
+ return Math.max(resultInt < 0 ? Integer.MAX_VALUE : resultInt, 1);
Review Comment:
fixed
--
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] Avoid negative estimated entry count [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1981719682
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3855,7 +3854,8 @@ static long estimateEntryCountBySize(long bytesSize, Position readPosition, Mana
posToRead = ml.getNextValidPosition(PositionFactory.create(posToRead.getLedgerId(), Long.MAX_VALUE));
}
}
- return Math.max(result, 1);
+ int resultInt = Long.valueOf(result).intValue();
+ return Math.max(resultInt < 0 ? Integer.MAX_VALUE : resultInt, 1);
Review Comment:
A better solution would be to check the long value before converting it, since in theory, it could "wrap around" multiple times.
```suggestion
int resultInt = result > Integer.MAX_VALUE ? Integer.MAX_VALUE : result;
return Math.max(resultInt, 1);
```
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982943547
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,9 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ long estimatedEntryCount = estimateEntryCountBySize(maxSizeBytes, readPosition, ledger);
+ if (estimatedEntryCount > Integer.MAX_VALUE) {
Review Comment:
I have a question that could `estimateEntryCountBySize` return a negative value? If so, should we check `estimatedEntryCount < 0` as well?
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1983064036
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,9 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ long estimatedEntryCount = estimateEntryCountBySize(maxSizeBytes, readPosition, ledger);
+ if (estimatedEntryCount > Integer.MAX_VALUE) {
Review Comment:
The estimateEntryCountBySize method returns this `return Math.max(result, 1);`, the minimum value is 1.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 closed pull request #24055: [fix] Avoid negative estimated entry count
URL: https://github.com/apache/pulsar/pull/24055
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982640666
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -5206,44 +5206,52 @@ public void testEstimateEntryCountBySize() throws Exception {
assertEquals(average3, 4 + BOOKKEEPER_READ_OVERHEAD_PER_ENTRY);
// Test: the individual ledgers.
- long entryCount1 =
+ int entryCount1 =
ManagedCursorImpl.estimateEntryCountBySize(average1 * 16, PositionFactory.create(ledger1, 0), ml);
assertEquals(entryCount1, 16);
- long entryCount2 =
+ int entryCount2 =
ManagedCursorImpl.estimateEntryCountBySize(average2 * 8, PositionFactory.create(ledger2, 0), ml);
assertEquals(entryCount2, 8);
- long entryCount3 =
+ int entryCount3 =
ManagedCursorImpl.estimateEntryCountBySize(average3 * 4, PositionFactory.create(ledger3, 0), ml);
assertEquals(entryCount3, 4);
// Test: across ledgers.
- long entryCount4 =
+ int entryCount4 =
ManagedCursorImpl.estimateEntryCountBySize((average1 * 100) + (average2 * 8), PositionFactory.create(ledger1, 0), ml);
assertEquals(entryCount4, 108);
- long entryCount5 =
+ int entryCount5 =
ManagedCursorImpl.estimateEntryCountBySize((average2 * 100) + (average3 * 4), PositionFactory.create(ledger2, 0), ml);
assertEquals(entryCount5, 104);
- long entryCount6 =
+ int entryCount6 =
ManagedCursorImpl.estimateEntryCountBySize((average1 * 100) + (average2 * 100) + (average3 * 4), PositionFactory.create(ledger1, 0), ml);
assertEquals(entryCount6, 204);
- long entryCount7 =
+ int entryCount7 =
ManagedCursorImpl.estimateEntryCountBySize((average1 * 20) + (average2 * 8), PositionFactory.create(ledger1, 80), ml);
assertEquals(entryCount7, 28);
- long entryCount8 =
+ int entryCount8 =
ManagedCursorImpl.estimateEntryCountBySize((average2 * 20) + (average3 * 4), PositionFactory.create(ledger2, 80), ml);
assertEquals(entryCount8, 24);
- long entryCount9 =
+ int entryCount9 =
ManagedCursorImpl.estimateEntryCountBySize((average1 * 20) + (average2 * 100) + (average3 * 4), PositionFactory.create(ledger1, 80), ml);
assertEquals(entryCount9, 124);
// Test: read more than entries written.
- long entryCount10 =
+ int entryCount10 =
ManagedCursorImpl.estimateEntryCountBySize((average1 * 100) + (average2 * 100) + (average3 * 100) + (average3 * 4) , PositionFactory.create(ledger1, 0), ml);
assertEquals(entryCount10, 304);
// cleanup.
ml.delete();
+
+ // test estimated long value convert to an int value
+ ml = (ManagedLedgerImpl) factory.open(mlName);
+ ml.addEntry(new byte[1000]);
+ int entryCount11 = ManagedCursorImpl.estimateEntryCountBySize(
+ Long.MAX_VALUE, PositionFactory.create(ml.getCurrentLedger().getId(), 0), ml);
Review Comment:
From my perspective, it seems to be an issue with https://github.com/apache/pulsar/pull/23931. In your test case, the managed ledger has only 1 ledger and the internal states are:
```yaml
- currentLedgerSize: 1000
currentLedgerEntries: 1
```
However, the `estimateEntryCountBySize` method call returns 8668582741404864, which is very unexpected.
It happens when `Long.MAX_VALUE` is passed to `estimateEntryCountBySize`, which means `applyMaxSizeCap` accepts `Long.MAX_VALUE`. In which case could it happen?
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982674766
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,12 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ return Math.min(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger), maxEntries);
Review Comment:
It's not a good practice to test an internal method but not a public method. Public methods can be called directly.
It's okay for us to remove the `estimateEntryCountBySize` and the related test in future, which could bring the regression again because we don't have tests for public methods like `applyMaxSizeCap` and `asyncReadEntries`.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982962416
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,9 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ long estimatedEntryCount = estimateEntryCountBySize(maxSizeBytes, readPosition, ledger);
+ if (estimatedEntryCount > Integer.MAX_VALUE) {
Review Comment:
This is a value estimated from `maxSizeBytes`, if it's invalid (either negative or a value bigger than a 32-bits integer), we should treat it as unlimited.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#issuecomment-2705477228
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/24055?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 74.20%. 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 [(`475af0b`)](https://app.codecov.io/gh/apache/pulsar/commit/475af0bf086504fa19595c7c54af56e549bf7eee?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 951 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/24055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #24055 +/- ##
============================================
+ Coverage 73.57% 74.20% +0.62%
+ Complexity 32624 2379 -30245
============================================
Files 1877 1861 -16
Lines 139502 144214 +4712
Branches 15299 16434 +1135
============================================
+ Hits 102638 107008 +4370
+ Misses 28908 28759 -149
- Partials 7956 8447 +491
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/24055/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/24055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.65% <50.00%> (+2.07%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/24055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.09% <50.00%> (-1.24%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/24055/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.72% <100.00%> (+0.88%)` | :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/24055?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...che/bookkeeper/mledger/impl/ManagedCursorImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/24055?src=pr&el=tree&filepath=managed-ledger%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbookkeeper%2Fmledger%2Fimpl%2FManagedCursorImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL01hbmFnZWRDdXJzb3JJbXBsLmphdmE=) | `79.62% <100.00%> (+0.32%)` | :arrow_up: |
... and [1055 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/24055/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>🚀 New features to boost your workflow: </summary>
- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- 📦 [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
Re: [PR] [fix] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982611559
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3855,7 +3854,8 @@ static long estimateEntryCountBySize(long bytesSize, Position readPosition, Mana
posToRead = ml.getNextValidPosition(PositionFactory.create(posToRead.getLedgerId(), Long.MAX_VALUE));
}
}
- return Math.max(result, 1);
+ int resultInt = Long.valueOf(result).intValue();
+ return Math.max(resultInt < 0 ? Integer.MAX_VALUE : resultInt, 1);
Review Comment:
> A better solution would be to check the long value before converting it, since in theory, it could "wrap around" multiple times.
I changed the method return type from long to int, because we still need to convert it from long to int when using it.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982659601
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,12 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ return Math.min(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger), maxEntries);
Review Comment:
Generally, we should take care of the type cast. It's intuitive to return a long for the result of `longVariable / intVariable`. But when comparing a long and an integer, we should not simply call `Long#intValue()` method.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#issuecomment-2702684945
> Just wondering how it could the result could exceed Integer.MAX_VALUE so that the integer value becomes negative in the current logic. Is it the line result += remainEntriesOfLedger; in estimateEntryCountBySize method that causes it? Could we also improve the existing logic and add some test cases to reproduce and prevent such regressions?
You can try to use this newly added case in the test.
```
var ml = (ManagedLedgerImpl) factory.open(mlName);
ml.addEntry(new byte[1000]);
int entryCount11 = ManagedCursorImpl.estimateEntryCountBySize(
Long.MAX_VALUE, PositionFactory.create(ml.getCurrentLedger().getId(), 0), ml);
```
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982735349
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,12 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ return Math.min(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger), maxEntries);
Review Comment:
Good idea, change it.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982654770
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,12 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ return Math.min(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger), maxEntries);
Review Comment:
I just checked the downstream side, which could call `asyncReadEntries(maxEntries, Long.MAX_VALUE, ...)` directly to only limit by `maxEntries`.
A more proper fix should be:
```java
final var estimateEntryCount = estimateEntryCountBySize(maxSizeBytes, readPosition, ledger);
if (estimateEntryCount >= Integer.MAX_VALUE) {
return maxEntries;
}
return Math.min((int) estimateEntryCount, maxEntries);
```
And we should add tests to **the public method** `applyMaxSizeCap` rather than the internal `estimateEntryCountBySize` method.
This PR changes the semantic of `estimateEntryCountBySize` that when the average size is greater than `Integer.MAX_VALUE`, it will return `Integer.MAX_VALUE`, which is anti-intuitive.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1982654770
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,12 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ return Math.min(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger), maxEntries);
Review Comment:
I just checked the downstream side, which could call `asyncReadEntries(maxEntries, Long.MAX_VALUE, ...)` directly to only limit by `maxEntries`.
A more proper and simple fix should be:
```java
final var estimateEntryCount = estimateEntryCountBySize(maxSizeBytes, readPosition, ledger);
if (estimateEntryCount >= Integer.MAX_VALUE) {
return maxEntries;
}
return Math.min((int) estimateEntryCount, maxEntries);
```
And we should add tests to **the public method** `applyMaxSizeCap` rather than the internal `estimateEntryCountBySize` method.
This PR changes the semantic of `estimateEntryCountBySize` that when the average size is greater than `Integer.MAX_VALUE`, it will return `Integer.MAX_VALUE`, which is anti-intuitive.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 closed pull request #24055: [fix] Avoid negative estimated entry count
URL: https://github.com/apache/pulsar/pull/24055
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 commented on code in PR #24055:
URL: https://github.com/apache/pulsar/pull/24055#discussion_r1983064036
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -3811,9 +3811,11 @@ public int applyMaxSizeCap(int maxEntries, long maxSizeBytes) {
if (maxSizeBytes == NO_MAX_SIZE_LIMIT) {
return maxEntries;
}
- int maxEntriesBasedOnSize =
- Long.valueOf(estimateEntryCountBySize(maxSizeBytes, readPosition, ledger)).intValue();
- return Math.min(maxEntriesBasedOnSize, maxEntries);
+ long estimatedEntryCount = estimateEntryCountBySize(maxSizeBytes, readPosition, ledger);
+ if (estimatedEntryCount > Integer.MAX_VALUE) {
Review Comment:
The estimateEntryCountBySize method returns this `return Math.max(result, 1);`, the value shouldn't be less than 1.
--
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] Avoid negative estimated entry count [pulsar]
Posted by "gaoran10 (via GitHub)" <gi...@apache.org>.
gaoran10 merged PR #24055:
URL: https://github.com/apache/pulsar/pull/24055
--
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