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 <chenhang@apache.org>
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java
index f7d2715..34a51ae 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java
@@ -835,27 +835,18 @@
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 968b5fc..85c3bca 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
@@ -350,8 +350,13 @@
// 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");
+ }
}
}
}
