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/04/10 05:52:32 UTC
[GitHub] [pulsar] horizonzy opened a new pull request, #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
horizonzy opened a new pull request, #15098:
URL: https://github.com/apache/pulsar/pull/15098
Master Issue: #5669 #5809
### Motivation
Relational code:
https://github.com/apache/pulsar/blob/af351647710a9f5787d3cdc04cf9376895476eda/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L48-L63
https://github.com/apache/pulsar/blob/af351647710a9f5787d3cdc04cf9376895476eda/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L2230-L2245
In #5809, there be 3 potential problem.
case 1: In `OpAddEntry` construction, it use `ManagedLedgerImpl#startReadOperationOnLedger` to setup readPosition, in startReadOperationOnLedger, it will callback OpAddEntry, but now some property not initial yet(like curtos,callback, entries...), cause npe.
case 2:
in startReadOperationOnLedger, the callback did't pass on ctx, the callback process need it, also case npe.
case 3:
in startReadOperationOnLedger, it didn't return when not found ledger id. The judgement unbox also case npe.
https://github.com/apache/pulsar/blob/af351647710a9f5787d3cdc04cf9376895476eda/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L2238
### Modifications
patch #5809
### Documentation
- [ ] `no-need-doc`
--
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] horizonzy commented on pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#issuecomment-1094204852
> Overall LGTM, but i have a question: Could we check for an `OpReadEntry` earlier (to avoid creating useless objects) or throw an exception (to achieve fail fast) when creating it? Please let me know what you think, thanks~
yep, I have thought it, but it's not atomic, it need user make invalid outside. This way more secure.
--
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 #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#issuecomment-1094186413
@horizonzy:Thanks for providing doc info!
--
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 #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#issuecomment-1094251033
There's also #12396 which fixes a NPE in OpReadEntry. That might be another problem that it's fixing.
--
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] horizonzy commented on a diff in pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#discussion_r846771186
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2230,9 +2236,8 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
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);
+ opReadEntry.makeInvalid();
Review Comment:
location 1:
It's useless code, I remove it at #15104. So needn't check it.
location 2:
It's a wait opReadEntry, when opAddEntry completed, it will nofity wait opReadEntry read again. The logicment is not conflict with this changes. And I fix the bad behavior when wait opReadEntry read at #15104
--
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 #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#issuecomment-1094186397
@horizonzy: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] Jason918 commented on a diff in pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#discussion_r846744916
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2230,9 +2236,8 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
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);
+ opReadEntry.makeInvalid();
Review Comment:
This changes the behavior of the method. So we need to go through all the usage of this method.
As far as I can see,
1. This line makes no sense any more. https://github.com/apache/pulsar/blob/a242f03f69791fcfa1934b6cab20689393228381/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L142
2. Also affects the `OpReadEntry.create` in https://github.com/apache/pulsar/blob/5cf3fa0d8050b16305ee060820b884d69ec8a828/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L765
--
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] horizonzy commented on a diff in pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#discussion_r847022328
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2230,9 +2236,8 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
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);
+ opReadEntry.makeInvalid();
Review Comment:
all done, pls check it again when you convenient.
--
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] horizonzy commented on pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#issuecomment-1094258921
> There's also #12396 which fixes a NPE in OpReadEntry. That might be another problem that it's fixing.
well, I will check it together.
--
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] horizonzy commented on a diff in pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#discussion_r846771457
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2230,9 +2236,11 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
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);
+ opReadEntry.makeInvalid();
+ return null;
+ } else {
+ // for wait opReadEntry, the readPosition will recalculate.
+ opReadEntry.makeValid();
Review Comment:
Here for wait opReadEntry, it maybe change from invalid to valid. So add this logicment.
--
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 #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#issuecomment-1166399351
The pr had no activity for 30 days, mark with Stale label.
--
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] horizonzy closed pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy closed pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
URL: https://github.com/apache/pulsar/pull/15098
--
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] horizonzy commented on a diff in pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#discussion_r846946744
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2230,9 +2236,8 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
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);
+ opReadEntry.makeInvalid();
Review Comment:
Add unit to cover wait opReadEntry case. And close #15102, the handle push at 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] mattisonchao commented on pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#issuecomment-1094206797
> > Overall LGTM, but i have a question: Could we check for an `OpReadEntry` earlier (to avoid creating useless objects) or throw an exception (to achieve fail fast) when creating it? Please let me know what you think, thanks~
>
> yep, I have thought it, but it's not atomic, it need user make invalid outside. This way more secure.
Got it. I was just wondering if there is a race condition here.
I would also like to say that we can throw an exception when we get an empty location. But it doesn't look particularly different from your approach.
--
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] horizonzy commented on a diff in pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#discussion_r846920806
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2230,9 +2236,8 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
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);
+ opReadEntry.makeInvalid();
Review Comment:
> Can you add a unit test to cover this path? It seems `readPosition` of this invalid OpReadEntry is used before `asyncReadEntries` in `hasMoreEntries`
Maybe not, it will still be used in asyncReadEntries.
At line_768, the wait OpReadEntry set to property `WAITING_READ_OP_UPDATER`.
https://github.com/apache/pulsar/blob/c1631588fd71f4e5d63557741bacf2b56f27563d/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L768
In notifyEntriesAvailable, get wait OpReadEntry from `WAITING_READ_OP_UPDATER`, and handle it to `asyncReadEntries `.
And in notifyEntriesAvailable, there be a bad behavior. fixes at #15102
https://github.com/apache/pulsar/blob/c1631588fd71f4e5d63557741bacf2b56f27563d/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L2749-L2772
--
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] horizonzy commented on a diff in pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#discussion_r846771186
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2230,9 +2236,8 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
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);
+ opReadEntry.makeInvalid();
Review Comment:
location 1:
It's useless code, I remove it at #15104. So needn't check it.
location 2:
It's a wait opReadEntry, when opAddEntry completed, it will nofity wait opReadEntry read again. The logicment is not conflict with this changes. And I fix the bad behavior when wait opReadEntry read at #15102
--
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 #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#issuecomment-1138075598
The pr had no activity for 30 days, mark with Stale label.
--
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] horizonzy commented on pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#issuecomment-1094206045
> Overall LGTM, but i have a question: Could we check for an `OpReadEntry` earlier (to avoid creating useless objects) or throw an exception (to achieve fail fast) when creating it? Please let me know what you think, thanks~
And the operation `startReadOperationOnLedger ` not only happen at opAddEntry construction, so I change the invalid in the 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
[GitHub] [pulsar] horizonzy commented on a diff in pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#discussion_r846759606
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2230,9 +2236,8 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
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);
+ opReadEntry.makeInvalid();
Review Comment:
Thanks for your reminder. I have checked it, found another problem, I will fix it at another pr.
This one patch just for npe.
--
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 a diff in pull request #15098: [fix][broker] patch #5809: Fix the ledgerID not found cause NPE.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #15098:
URL: https://github.com/apache/pulsar/pull/15098#discussion_r846911373
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2230,9 +2236,8 @@ void updateCursor(ManagedCursorImpl cursor, PositionImpl newPosition) {
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
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);
+ opReadEntry.makeInvalid();
Review Comment:
> It's a wait opReadEntry, when opAddEntry completed, it will nofity wait opReadEntry read again.
Can you add a unit test to cover this path?
It seems `readPosition` of this invalid OpReadEntry is used before `asyncReadEntries` in `hasMoreEntries`
--
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