Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,9 @@ public void asyncReadEntries(int numberOfEntriesToRead, long maxSizeBytes, ReadE

PENDING_READ_OPS_UPDATER.incrementAndGet(this);
OpReadEntry op = OpReadEntry.create(this, readPosition, numOfEntriesToRead, callback, ctx, maxPosition);
ledger.asyncReadEntries(op);
if (op != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please provide more information about why op could be null?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ledger.asyncReadEntries(op);
}
}

@Override
Expand Down Expand Up @@ -763,6 +765,9 @@ public void asyncReadEntriesOrWait(int maxEntries, long maxSizeBytes, ReadEntrie
} else {
OpReadEntry op = OpReadEntry.create(this, readPosition, numberOfEntriesToRead, callback,
ctx, maxPosition);
if (op == null) {
return;
}

if (!WAITING_READ_OP_UPDATER.compareAndSet(this, null, op)) {
callback.readEntriesFailed(new ManagedLedgerException("We can only have a single waiting callback"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still npe, can't pass on null ctx, in the process of callback, need use it.

return null;
}

if (ledgerId != position.getLedgerId()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ class OpReadEntry implements ReadEntriesCallback {
public static OpReadEntry create(ManagedCursorImpl cursor, PositionImpl readPositionRef, int count,
ReadEntriesCallback callback, Object ctx, PositionImpl maxPosition) {
OpReadEntry op = RECYCLER.get();
op.readPosition = cursor.ledger.startReadOperationOnLedger(readPositionRef, op);
op.cursor = cursor;
op.count = count;
op.callback = callback;
Expand All @@ -58,6 +57,11 @@ public static OpReadEntry create(ManagedCursorImpl cursor, PositionImpl readPosi
}
op.maxPosition = maxPosition;
op.ctx = ctx;
PositionImpl position = cursor.ledger.startReadOperationOnLedger(readPositionRef, op);
if (position == null) {
return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can recycle this OpReadEntry if we return null.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is :

  • readEntriesComplete or readEntriesFailed must be called in any case, so read event would always receive response.
  • OpReadEntry should be recyle in readEntriesComplete or readEntriesFailed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to skip recycling of failed entries. That was something that was recently removed for OpAddEntry instances in #12993.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
op.readPosition = position;
op.nextReadPosition = PositionImpl.get(op.readPosition);
return op;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3393,4 +3393,31 @@ public void testCancellationOfScheduledTasks() throws Exception {
assertTrue(timeoutTask2.isCancelled());
assertTrue(checkLedgerRollTask2.isCancelled());
}

@Test(timeOut = 20000)
public void testReadNonExistentLedger() throws Exception {
ManagedLedgerConfig config = new ManagedLedgerConfig();

ManagedLedgerImpl ledger = (ManagedLedgerImpl) factory.open("test-non-existent-ledger", config);
ManagedCursorImpl cursor = (ManagedCursorImpl) ledger.openCursor("test-non-existent-cursor");

CompletableFuture<Boolean> future = new CompletableFuture<>();
PositionImpl pos = PositionImpl.latest;
cursor.seek(pos);
cursor.asyncReadEntries(1, new ReadEntriesCallback() {
@Override
public void readEntriesComplete(List<Entry> entries, Object ctx) {
}

@Override
public void readEntriesFailed(ManagedLedgerException exception, Object ctx) {
future.complete(true);
}
}, null, pos);

cursor.close();
ledger.close();
assert(future.get());
assertEquals(cursor.getPendingReadOpsCount(), 0);
}
}