Fix NPE when create OpEntry#12396
Conversation
| PENDING_READ_OPS_UPDATER.incrementAndGet(this); | ||
| OpReadEntry op = OpReadEntry.create(this, readPosition, numOfEntriesToRead, callback, ctx, maxPosition); | ||
| ledger.asyncReadEntries(op); | ||
| if (op != null) { |
There was a problem hiding this comment.
Could you please provide more information about why op could be null?
There was a problem hiding this comment.
@codelipenghui This PR is used to solve NPE, is a copy of #11813 .
If op is null, the read faild event would be trigger.
There is another solution, see #11292 .
There was a problem hiding this comment.
It will be very dangerous if the dispatcher call read entries but can't receive any response, I think the broker will stop dispatching messages.
|
@codelipenghui Do you think my solution is suitable? By the way, I add test. |
|
/pulsarbot run-failure-checks |
| op.ctx = ctx; | ||
| PositionImpl position = cursor.ledger.startReadOperationOnLedger(readPositionRef, op); | ||
| if (position == null) { | ||
| return null; |
There was a problem hiding this comment.
We can recycle this OpReadEntry if we return null.
There was a problem hiding this comment.
My point is :
readEntriesCompleteorreadEntriesFailedmust be called in any case, so read event would always receive response.OpReadEntryshould be recyle inreadEntriesCompleteorreadEntriesFailed.
There was a problem hiding this comment.
It might be better to skip recycling of failed entries. That was something that was recently removed for OpAddEntry instances in #12993.
There was a problem hiding this comment.
yes, I guess readEntriesFailed would have to be called before returning null so that the comment by @codelipenghui in https://github.com/apache/pulsar/pull/12396/files#r754321070 could be addressed.
|
Please revert the change of |
|
The pr had no activity for 30 days, mark with Stale label. |
|
@casuallc Please rebase the changes |
| @@ -2154,6 +2154,7 @@ PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opRea | |||
| 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); | |||
There was a problem hiding this comment.
still npe, can't pass on null ctx, in the process of callback, need use it.
|
here also need handle, this change maybe not cover all situation. coud you see #15098, maybe it's better to fix problem. |
|
Closed as stale and conflicts. |
|
Fixed by #15837 |
8 participants
Fixes #11796
No need doc.