This is an automated email from the ASF dual-hosted git repository. yong pushed a commit to branch branch-4.15 in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
commit f9e4a3b5b68f7505d7ed472d79bfaf973efa55ea Author: Jack Vanlightly <[email protected]> AuthorDate: Mon Aug 1 09:11:35 2022 +0200 Issue #2908: Replace unsafe NoEntryException with IOException (#2909) * Replace unsafe NoEntryException with IOException Throwing a NoEntryException from the entry logger for an entry that the index says should exist is unsafe. It can cause ledger truncation during ledger recovery. It only takes a single false NoSuchEntry response to trigger truncation. NoEntryException should only be thrown from inside ledger storage if the entry is not found in the index. * fix CI Co-authored-by: chenhang <[email protected]> (cherry picked from commit 966b865973714c1bde596004bc4e5c1daa5a6d00) --- .../java/org/apache/bookkeeper/bookie/EntryLogger.java | 17 ++++------------- .../org/apache/bookkeeper/bookie/LedgerCacheTest.java | 9 +++++++-- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java index 859f3e588b..d69e434ef5 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java @@ -850,27 +850,18 @@ public class EntryLogger { if (validateEntry) { validateEntry(ledgerId, entryId, entryLogId, pos, sizeBuff); } - } catch (EntryLookupException.MissingEntryException entryLookupError) { - throw new Bookie.NoEntryException("Short read from entrylog " + entryLogId, - ledgerId, entryId); } catch (EntryLookupException e) { - throw new IOException(e.toString()); + throw new IOException("Bad entry read from log file id: " + entryLogId, e); } ByteBuf data = allocator.buffer(entrySize, entrySize); int rc = readFromLogChannel(entryLogId, fc, data, pos); if (rc != entrySize) { - // Note that throwing NoEntryException here instead of IOException is not - // without risk. If all bookies in a quorum throw this same exception - // the client will assume that it has reached the end of the ledger. - // However, this may not be the case, as a very specific error condition - // could have occurred, where the length of the entry was corrupted on all - // replicas. However, the chance of this happening is very very low, so - // returning NoEntryException is mostly safe. data.release(); - throw new Bookie.NoEntryException("Short read for " + ledgerId + "@" + throw new IOException("Bad entry read from log file id: " + entryLogId, + new EntryLookupException("Short read for " + ledgerId + "@" + entryId + " in " + entryLogId + "@" - + pos + "(" + rc + "!=" + entrySize + ")", ledgerId, entryId); + + pos + "(" + rc + "!=" + entrySize + ")")); } data.writerIndex(entrySize); diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/LedgerCacheTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/LedgerCacheTest.java index ba8e1b96d0..15947977c7 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/LedgerCacheTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/LedgerCacheTest.java @@ -352,8 +352,13 @@ public class LedgerCacheTest { // this is fine, means the ledger was written to the index cache, but not // the entry log } catch (IOException ioe) { - LOG.info("Shouldn't have received IOException", ioe); - fail("Shouldn't throw IOException, should say that entry is not found"); + if (ioe.getCause() instanceof DefaultEntryLogger.EntryLookupException) { + // this is fine, means the ledger was not fully written to + // the entry log + } else { + LOG.info("Shouldn't have received IOException for entry {}", i, ioe); + fail("Shouldn't throw IOException, should say that entry is not found"); + } } } }
