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/06/02 00:28:19 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request, #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
mattisonchao opened a new pull request, #15837:
URL: https://github.com/apache/pulsar/pull/15837
### Motivation
In current implementation, We have more than one potential NPE relate `ManagedLedgerImpl#startReadOperationOnLedger` method.
**1. Unbox NPE**
https://github.com/apache/pulsar/blob/b13d15c4d6d795f33c6ed23f920fabbb3eeaf745/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L2230-L2244
According to this code, we can know that when the ledger id is null, we will fail `OpReadEntry`. However, there is no direct return here after failure. NPE will be thrown on line 2238. Because we want to unbox `null`.
**2. null `OpReadEntry` context**
According to the code above, we can see when we fail `OpReadEntry`, we pass a null value as context(line 2235). there is an NPE when the dispatcher gets the callback.
https://github.com/apache/pulsar/blob/0975cdc2489739b7518ee7acc78bbc6e8c8e2e4d/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherSingleActiveConsumer.java#L478-L484
**3. `OpReadEntry#create`**
https://github.com/apache/pulsar/blob/93761284b9f6875da0403f5fedb6ccbfbbcd7315/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L48-L63
When we create `OpReadEntry` and then call `ManagedCursor#startReadOperationOnLedger` , assuming the ledger id is equal to null. At this point, we will fail this `OpReadEntry`. But at the current time, other parameters are not initialized. When we call `OpReadEntry#readEntriesFailed`. The cursor will be null and the NPE will be thrown.
https://github.com/apache/pulsar/blob/93761284b9f6875da0403f5fedb6ccbfbbcd7315/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L90-L94
### Modifications
- Remove fail `OpReadEntry` logic in `ManagedCursor#startReadOperationOnLedger`, when ledger id equals null, we can return the original value. the `ManagedLedger` will validate if this operation is legal.
From the perspective of the overall design, the `OpReadEntry` is just a middle state object, that may have illegal value, we have to check this `OpReadEntry` is valid in `ManagedLedger#asyncReadEntries`(Current we already do that).
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `doc-not-needed`
(Please explain why)
--
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] eolivelli merged pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
eolivelli merged PR #15837:
URL: https://github.com/apache/pulsar/pull/15837
--
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] mattisonchao commented on pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#issuecomment-1148120928
After discussing with @Technoboy-, we may find why the `ledgers.ceilingKey` get `Null`; please consider this condition:
### Precondition
- We have a ledger 12222:-1 at the `ManagedLedger` `ledgers`.
- We have an inactive cursor that read position is `12222:0`.
### Cases
1. When a new producer wants to produce a message on the topic but adds an entry to BK failed. It will prepare to close the current ledger and create a new one. because the current ledger entries are 0. So, we decided to delete it immediately. The logic shows below:
https://github.com/apache/pulsar/blob/5455f4ddd0ff7b67b728958f7a52cb665a73c20e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1618-L1659
**After line:1643, we may have open ledgers now.**
2. At mealtimes, the new subscription wants to read messages and create the `OpReadEntry`. but the `ledgers` are empty then NPE occur at `startReadOperationOnLedger` method.
/cc @Technoboy- @codelipenghui
--
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] mattisonchao commented on pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#issuecomment-1148153085
@gaoran10 Got it, I will do that.
--
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] mattisonchao closed pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
mattisonchao closed pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
URL: https://github.com/apache/pulsar/pull/15837
--
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- commented on a diff in pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#discussion_r890843431
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2227,15 +2227,9 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
}
}
- PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
+ PositionImpl startReadOperationOnLedger(PositionImpl position) {
Long ledgerId = ledgers.ceilingKey(position.getLedgerId());
- if (null == ledgerId) {
- opReadEntry.readEntriesFailed(new ManagedLedgerException.NoMoreEntriesToReadException("The ceilingKey(K key"
- + ") method is used to return the least key greater than or equal to the given key, "
- + "or null if there is no such key"), null);
- }
-
- if (ledgerId != position.getLedgerId()) {
+ if (ledgerId != null && ledgerId != position.getLedgerId()) {
Review Comment:
I have found out the root cause why ledgerId is null, so we don't need to add log here.
--
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] mattisonchao commented on pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#issuecomment-1148247518
> Maybe we could add a unit test to reproduce this problem, for example reading a position greater than max ledger id exists in `ManagedLedgerImpl#ledgers`? Or the `ledgers` is an empty set.
Done, PTAL.
--
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] HQebupt commented on pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
HQebupt commented on PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#issuecomment-1141220812
/pulsarbot run-failure-checks
--
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] Jason918 commented on pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#issuecomment-1207323246
move `release/2.7.5` to https://github.com/apache/pulsar/pull/16966
--
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- commented on a diff in pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#discussion_r887441337
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2227,15 +2227,9 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
}
}
- PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
+ PositionImpl startReadOperationOnLedger(PositionImpl position) {
Long ledgerId = ledgers.ceilingKey(position.getLedgerId());
- if (null == ledgerId) {
- opReadEntry.readEntriesFailed(new ManagedLedgerException.NoMoreEntriesToReadException("The ceilingKey(K key"
- + ") method is used to return the least key greater than or equal to the given key, "
- + "or null if there is no such key"), null);
- }
-
- if (ledgerId != position.getLedgerId()) {
+ if (ledgerId != null && ledgerId != position.getLedgerId()) {
Review Comment:
The case where the ledgerId is null is really weird. We reconfirmed the logic. Although there is no problem in returning the position, do we need to add a log here?
@codelipenghui
--
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] github-actions[bot] commented on pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#issuecomment-1140974075
@mattisonchao:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? 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
[GitHub] [pulsar] gaoran10 commented on pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#issuecomment-1148138492
Maybe we could add a unit test to reproduce this problem, for example reading a position greater than max ledger id exists in `ManagedLedgerImpl#ledgers`?
--
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] mattisonchao commented on pull request #15837: [Fix][broker] Fix NPE when ledger id not found in `OpReadEntry`
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15837:
URL: https://github.com/apache/pulsar/pull/15837#issuecomment-1148123399
And because the `ManagedLedger` has the state to check if we can do something. So, I think the null check at `OpReadEntry` is enough.
--
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