[bookkeeper] branch master updated: Issue #2908: Replace unsafe NoEntryException with IOException (#2909)

[ch...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

chenhang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git


The following commit(s) were added to refs/heads/master by this push:
     new 966b865973 Issue #2908: Replace unsafe NoEntryException with IOException (#2909)
966b865973 is described below

commit 966b865973714c1bde596004bc4e5c1daa5a6d00
Author: Jack Vanlightly <va...@gmail.com>
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 <ch...@apache.org>
---
 .../apache/bookkeeper/bookie/DefaultEntryLogger.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/DefaultEntryLogger.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java
index f7d2715b7d..34a51aefbb 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 @@ public class DefaultEntryLogger implements 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 968b5fce58..85c3bca8d7 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 @@ 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");
+                }
             }
         }
     }