Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Handle unknown runtime exception while reading entries (#2993)" #18280

Closed
wants to merge 1 commit into from
Closed

Revert "Handle unknown runtime exception while reading entries (#2993)" #18280

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 1 addition & 29 deletions ...ed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/RangeEntryCacheImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,20 +201,6 @@ public void invalidateAllEntries(long ledgerId) {
@Override
public void asyncReadEntry(ReadHandle lh, PositionImpl position, final ReadEntryCallback callback,
final Object ctx) {
try {
asyncReadEntry0(lh, position, callback, ctx);
} catch (Throwable t) {
log.warn("failed to read entries for {}-{}", lh.getId(), position, t);
Copy link
Member Author

@mattisonchao mattisonchao Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If callback.readEntryComplete(cachedEntry, ctx); throw any exception(line 226), the exception will also be catched by here, and invoke the callback.readEntryFailed(createManagedLedgerException(t), ctx);.

This means that in some cases we may call both methods, which is a risk to some recycled objects or other states.

Copy link
Member Author

Choose a reason for hiding this comment

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

And we shouldn't expire the cache for callback exceptions.

// invalidate all entries related to ledger from the cache (it might happen if entry gets corrupt
// (entry.data is already deallocate due to any race-condition) so, invalidate cache and next time read from
// the bookie)
invalidateAllEntries(lh.getId());
callback.readEntryFailed(createManagedLedgerException(t), ctx);
}
}

private void asyncReadEntry0(ReadHandle lh, PositionImpl position, final ReadEntryCallback callback,
final Object ctx) {
if (log.isDebugEnabled()) {
log.debug("[{}] Reading entry ledger {}: {}", ml.getName(), lh.getId(), position.getEntryId());
}
Expand Down Expand Up @@ -254,22 +240,8 @@ private void asyncReadEntry0(ReadHandle lh, PositionImpl position, final ReadEnt
}

@Override
public void asyncReadEntry(ReadHandle lh, long firstEntry, long lastEntry, boolean shouldCacheEntry,
final ReadEntriesCallback callback, Object ctx) {
try {
asyncReadEntry0(lh, firstEntry, lastEntry, shouldCacheEntry, callback, ctx);
} catch (Throwable t) {
log.warn("failed to read entries for {}--{}-{}", lh.getId(), firstEntry, lastEntry, t);
// invalidate all entries related to ledger from the cache (it might happen if entry gets corrupt
// (entry.data is already deallocate due to any race-condition) so, invalidate cache and next time read from
// the bookie)
invalidateAllEntries(lh.getId());
callback.readEntriesFailed(createManagedLedgerException(t), ctx);
}
}

@SuppressWarnings({ "unchecked", "rawtypes" })
void asyncReadEntry0(ReadHandle lh, long firstEntry, long lastEntry, boolean shouldCacheEntry,
public void asyncReadEntry(ReadHandle lh, long firstEntry, long lastEntry, boolean shouldCacheEntry,
final ReadEntriesCallback callback, Object ctx) {
final long ledgerId = lh.getId();
final int entriesToRead = (int) (lastEntry - firstEntry) + 1;
Expand Down