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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
