Re: [PR] Fix DbLedgerStorage checkpoint losing ledger metadata when write cache is empty [bookkeeper]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #4843:
URL: https://github.com/apache/bookkeeper/pull/4843#discussion_r3608627610


##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java:
##########
@@ -232,6 +247,57 @@ public void simple() throws Exception {
         }
     }
 
+    @Test
+    public void testFlushPersistsFencedMetadataWithoutPendingEntries() throws Exception {
+        long ledgerId = 1;
+        addEntryAndFlush(ledgerId, 0);
+
+        assertTrue(storage.setFenced(ledgerId));
+        assertFalse(storage.isFlushRequired());
+
+        storage.flush();
+        storage.shutdown();
+
+        Bookie restartedBookie = new TestBookieImpl(conf);
+        DbLedgerStorage restartedStorage = (DbLedgerStorage) restartedBookie.getLedgerStorage();
+        try {
+            assertTrue(restartedStorage.isFenced(ledgerId));
+        } finally {
+            restartedStorage.shutdown();
+        }
+
+        storage = (DbLedgerStorage) new TestBookieImpl(conf).getLedgerStorage();
+    }
+
+    @Test
+    public void testFlushPersistsExplicitLacMetadataWithoutPendingEntries() throws Exception {
+        long ledgerId = 1;
+        addEntryAndFlush(ledgerId, 0);
+
+        ByteBuf explicitLac = Unpooled.buffer(Long.BYTES * 2);
+        explicitLac.writeLong(ledgerId);
+        explicitLac.writeLong(0);
+        storage.setExplicitLac(ledgerId, explicitLac);
+        assertFalse(storage.isFlushRequired());
+
+        storage.flush();
+        storage.shutdown();
+
+        Bookie restartedBookie = new TestBookieImpl(conf);
+        DbLedgerStorage restartedStorage = (DbLedgerStorage) restartedBookie.getLedgerStorage();
+        ByteBuf recoveredExplicitLac = null;
+        try {
+            recoveredExplicitLac = restartedStorage.getExplicitLac(ledgerId);
+            Assert.assertNotNull(recoveredExplicitLac);
+            assertEquals(0, ByteBufUtil.compare(explicitLac, recoveredExplicitLac));
+        } finally {
+            ReferenceCountUtil.release(recoveredExplicitLac);
+            restartedStorage.shutdown();
+        }

Review Comment:
   The test allocates an explicitLac ByteBuf but never releases it, and the restarted TestBookieImpl is not shut down. Since setExplicitLac copies the bytes, the caller still owns the buffer and should release it; shutting down the Bookie also ensures journals/threads are cleaned up.



##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -807,6 +807,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
 
         try {
             if (writeCache.isEmpty()) {
+                // Ledger metadata updates can be journaled without any pending entry data.
+                // Persist them before allowing the journal checkpoint mark to advance.
+                flushLedgerIndex();
+                lastCheckpoint = thisCheckpoint;

Review Comment:
   The metadata-only early return only checks writeCache, but checkpoint flushes from writeCacheBeingFlushed. If a previous flush swapped caches and then failed, writeCache can be empty while writeCacheBeingFlushed still contains unpersisted entries. In that state, this branch would flush ledger metadata, advance lastCheckpoint, and return without persisting entry data, which can allow journal checkpoints to advance past unflushed entries.



##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java:
##########
@@ -232,6 +247,57 @@ public void simple() throws Exception {
         }
     }
 
+    @Test
+    public void testFlushPersistsFencedMetadataWithoutPendingEntries() throws Exception {
+        long ledgerId = 1;
+        addEntryAndFlush(ledgerId, 0);
+
+        assertTrue(storage.setFenced(ledgerId));
+        assertFalse(storage.isFlushRequired());
+
+        storage.flush();
+        storage.shutdown();
+
+        Bookie restartedBookie = new TestBookieImpl(conf);
+        DbLedgerStorage restartedStorage = (DbLedgerStorage) restartedBookie.getLedgerStorage();
+        try {
+            assertTrue(restartedStorage.isFenced(ledgerId));
+        } finally {
+            restartedStorage.shutdown();
+        }

Review Comment:
   The restarted TestBookieImpl is never shut down. Shutting down only the LedgerStorage can leave journals/threads running and can make tests flaky/leaky; prefer shutting down the Bookie instance you created.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org