Fix DbLedgerStorage checkpoint losing ledger metadata when write cache is empty#4843
Open
void-ptr974 wants to merge 1 commit into
Open
Fix DbLedgerStorage checkpoint losing ledger metadata when write cache is empty#4843void-ptr974 wants to merge 1 commit into
void-ptr974 wants to merge 1 commit into
Conversation
void-ptr974
changed the title
void-ptr974
changed the title
void-ptr974
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes a durability gap in SingleDirectoryDbLedgerStorage.checkpoint() where an empty write cache caused an early return that could skip persisting ledger-metadata updates (e.g., fenced state, explicitLac), potentially allowing journal checkpoint advancement before those metadata changes were flushed.
Changes:
- Flush ledger metadata index even when the write cache is empty during checkpoint.
- Ensure shard checkpoint (
lastCheckpoint) is advanced after metadata-only flush. - Add regression tests covering fenced state and explicitLac persistence across flush/restart with no pending entries.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java | Ensures ledger metadata index is flushed during metadata-only checkpoints; refactors ledger index flush into a helper. |
| bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java | Adds regression tests for fenced and explicitLac metadata persistence when no entry data is pending. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
809
to
+813
| 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; |
Comment on lines
+261
to
+267
| Bookie restartedBookie = new TestBookieImpl(conf); | ||
| DbLedgerStorage restartedStorage = (DbLedgerStorage) restartedBookie.getLedgerStorage(); | ||
| try { | ||
| assertTrue(restartedStorage.isFenced(ledgerId)); | ||
| } finally { | ||
| restartedStorage.shutdown(); | ||
| } |
Comment on lines
+277
to
+296
| 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(); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
DbLedgerStorage checkpoint returned early when the write cache was empty, which skipped ledgerIndex.flush(). Ledger metadata updates such as fenced state and explicitLac can be journaled without pending entry data, so checkpointComplete could advance the journal lastMark before those metadata updates were persisted.
Changes
Tests