Skip to content

Issue 4900: Read LAC from EntryMemTable in SortedLedgerStorage if it is missing from the Ledger Cache #4902

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
djsweet wants to merge 3 commits into
base: master
Choose a base branch
from
+212 −14
Open

Issue 4900: Read LAC from EntryMemTable in SortedLedgerStorage if it is missing from the Ledger Cache #4902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0754894
EntryMemTable now exposes a getLastAddConfirmed method
djsweet Sep 23, 2026
7a19d7d
Expose getLastAddConfirmedFromLedgerEntry in InterleavedLedgerStorage
djsweet Sep 24, 2026
f1b55a1
Consult the EntryMemTable in SortedLedgerStorage.getLastAddConfirmed
djsweet Sep 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryMemTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

package org.apache.bookkeeper.bookie;

import io.netty.buffer.ByteBuf;
import io.netty.util.ReferenceCountUtil;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.Iterator;
Expand Down Expand Up @@ -441,6 +443,26 @@ public EntryKeyValue getLastEntry(long ledgerId) throws IOException {
return (EntryKeyValue) result;
}

/**
Gets the lastAddConfirmed for a ledger from its highest available entry.

Returns null if there are no entries for the given ledger
*/
public Long getLastAddConfirmed(long ledgerId) throws IOException {
Long lac = null;
EntryKeyValue ekv = getLastEntry(ledgerId);
if (ekv != null) {

@codelipenghui codelipenghui Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description mentions #4895 "when ledger metadata is in use". In the stock write path, METAENTRY_ID_* records only go to the journal and never reach LedgerStorage.addEntry, so I don't think this PR depends on #4895 there.

If negative entry IDs can reach the memtable in your setup, though, getLastEntry may return such an entry. This method would then read bytes 16–24 of a meta payload as the LAC, which gives a bogus and potentially higher (unsafe) value. A cheap guard: if (ekv != null && ekv.getEntryId() >= 0).

ByteBuf bb = ekv.getValueAsByteBuffer();
try {
bb.skipBytes(2 * Long.BYTES); // Skip ledger and entry id
lac = bb.readLong();
} finally {
ReferenceCountUtil.release(bb);
}
}
return lac;
}

/**
* Check if the entire heap usage for this EntryMemTable exceeds limit.
*/
Expand Down
6 changes: 6 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexPersistenceMgr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,4 +722,10 @@ public LedgerCache.LedgerIndexMetadata readLedgerIndexMetadata(long ledgerId) th
}
}
}

@VisibleForTesting
void invalidateLedger(long ledgerId) {
this.writeFileInfoCache.invalidate(ledgerId);
this.readFileInfoCache.invalidate(ledgerId);
}
}
32 changes: 21 additions & 11 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import static org.apache.bookkeeper.bookie.BookKeeperServerStats.STORAGE_SCRUB_PAGES_SCANNED;
import static org.apache.bookkeeper.bookie.BookKeeperServerStats.STORAGE_SCRUB_PAGE_RETRIES;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.RateLimiter;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
Expand Down Expand Up @@ -360,17 +361,21 @@ public boolean entryExists(long ledgerId, long entryId) throws IOException {
public long getLastAddConfirmed(long ledgerId) throws IOException {
Long lac = ledgerCache.getLastAddConfirmed(ledgerId);
if (lac == null) {
ByteBuf bb = getEntry(ledgerId, BookieProtocol.LAST_ADD_CONFIRMED);
if (null == bb) {
return BookieProtocol.INVALID_ENTRY_ID;
} else {
try {
bb.skipBytes(2 * Long.BYTES); // skip ledger & entry id
lac = bb.readLong();
lac = ledgerCache.updateLastAddConfirmed(ledgerId, lac);
} finally {
ReferenceCountUtil.release(bb);
}
lac = getLastAddConfirmedFromLedgerEntry(ledgerId);
}
return lac;
}

public long getLastAddConfirmedFromLedgerEntry(long ledgerId) throws IOException {

@codelipenghui codelipenghui Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: SortedLedgerStorage is in the same package, so this can be package-private instead of widening the public API of InterleavedLedgerStorage.

A short Javadoc would help too:

  • it updates the ledger cache as a side effect;
  • when nothing is flushed it throws NoEntryException rather than returning INVALID_ENTRY_ID.

getEntry never returns null, so the null != bb branch is unreachable.

long lac = BookieProtocol.INVALID_ENTRY_ID;
ByteBuf bb = getEntry(ledgerId, BookieProtocol.LAST_ADD_CONFIRMED);
if (null != bb) {
try {
bb.skipBytes(2 * Long.BYTES);
lac = bb.readLong();
lac = ledgerCache.updateLastAddConfirmed(ledgerId, lac);
} finally {
ReferenceCountUtil.release(bb);
}
}
return lac;
Expand Down Expand Up @@ -726,4 +731,9 @@ public void clearStorageStateFlag(StorageState flags) throws IOException {
throw new UnsupportedOperationException(
"Storage state flags only supported for DbLedgerStorage");
}

@VisibleForTesting
void removeLedgerCacheEntry(long ledgerId) {

@codelipenghui codelipenghui Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: these test-only hooks widen four production classes:

  • IndexPersistenceMgr.invalidateLedger
  • LedgerCacheImpl.invalidateLedger
  • this method, with the LedgerCacheImpl downcast
  • SortedLedgerStorage.removeLedgerCacheEntry

The tests already live in org.apache.bookkeeper.bookie, so they can invalidate through the package-private IndexPersistenceMgr.writeFileInfoCache/readFileInfoCache, as IndexPersistenceMgrTest does. Even better: conf.setOpenFileLimit(1) and write more than 2 * openFileLimit ledgers. That exercises the real eviction path end to end without any hooks.

((LedgerCacheImpl) ledgerCache).invalidateLedger(ledgerId);
}
}
6 changes: 6 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCacheImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

package org.apache.bookkeeper.bookie;

import com.google.common.annotations.VisibleForTesting;
import io.netty.buffer.ByteBuf;
import java.io.IOException;
import java.util.Collections;
Expand Down Expand Up @@ -233,4 +234,9 @@ public long nextLong() {
}
};
}

@VisibleForTesting
void invalidateLedger(long ledgerId) {
indexPersistenceManager.invalidateLedger(ledgerId);
}
}
21 changes: 20 additions & 1 deletion bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,21 @@ public ByteBuf getEntry(long ledgerId, long entryId) throws IOException, BookieE

@Override
public long getLastAddConfirmed(long ledgerId) throws IOException {
return interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
LedgerCache lc = interleavedLedgerStorage.getLedgerCache();
Long lac = lc.getLastAddConfirmed(ledgerId);

@codelipenghui codelipenghui Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about the root cause in the description. I reproduced #4900 with 1 bookie, openFileLimit=4 and 24 single-entry ledgers. On master the read fails with NoEntryException, which becomes ENOENTRY (BKNoSuchEntryException), not NoLedgerException/ENOLEDGER.

When the FileInfo is evicted from both caches, FileInfo.close(true) writes a header-only index file. So this call succeeds and returns null, which is exactly the case this PR fixes. The failure also only starts above about 2×openFileLimit, because the read cache (2×openFileLimit) keeps the FileInfo alive until then.

If there really is a path where getFileInfo throws NoLedgerException for a ledger that only lives in the memtable, it would propagate from this line before the memtable is consulted, and this PR wouldn't fix it. Could you update the description and the issue, or point to that path? If it does exist, catching it here needs care not to resurrect a LAC for a deleted ledger whose entries are still in the memtable.

if (null == lac) {
// If the last add confirmed has fallen out of the ledger cache,
// it may be in the memTable, and more importantly, it may be
// in the memTable but _not_ in ledger storage.
lac = memTable.getLastAddConfirmed(ledgerId);
if (null != lac) {
lc.updateLastAddConfirmed(ledgerId, lac);

@codelipenghui codelipenghui Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateLastAddConfirmed returns the effective (max) LAC. If an addEntry or setExplicitLac raced in between line 244 and here, the FileInfo may already hold a higher value. Suggest lac = lc.updateLastAddConfirmed(ledgerId, lac);, which matches InterleavedLedgerStorage.getLastAddConfirmedFromLedgerEntry.

}
}
if (null == lac) {
lac = interleavedLedgerStorage.getLastAddConfirmedFromLedgerEntry(ledgerId);
}
return lac;

@codelipenghui codelipenghui Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, could be a follow-up: waitForLastAddConfirmedUpdate (just below) still only looks at FileInfo.lac.

After eviction, waitForLastAddConfirmedUpdate(ledgerId, -1, watcher) returns true, which parks the long poll, even though the memtable LAC is 0. It only returns false once something has called getLastAddConfirmed. So a long poll that is the first request after eviction waits out its full timeout; with this PR it then answers correctly.

Consider calling getLastAddConfirmed(ledgerId) first in SortedLedgerStorage.waitForLastAddConfirmedUpdate to refill the cached LAC, or track this in a follow-up issue.

}

@Override
Expand Down Expand Up @@ -461,4 +475,9 @@ public void clearStorageStateFlag(StorageState flags) throws IOException {
throw new UnsupportedOperationException(
"Storage state flags only supported for DbLedgerStorage");
}

@VisibleForTesting
void removeLedgerCacheEntry(long ledgerId) {
interleavedLedgerStorage.removeLedgerCacheEntry(ledgerId);
}
}
29 changes: 29 additions & 0 deletions bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryMemTableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,5 +468,34 @@ public void testAddSameEntries() throws IOException {
assertEquals(memTable.kvmap.size(), 0);
assertEquals(memTable.skipListSemaphore.availablePermits(), initialPermits);
}

@Test
public void testGetLastAddConfirmed() throws IOException {

@codelipenghui codelipenghui Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also cover:

  • a ledger with no entries returning null;
  • the snapshot path: call memTable.snapshot() and then add more entries, so the last entry is looked up across kvmap and snapshot.

Minor: the payload's ledger id (offset 0) is 0, while the key is ledger 1.

final long ledgerId = 1;
final byte[] bytes = new byte[3 * Long.BYTES];
final ByteBuffer bb = ByteBuffer.wrap(bytes);

bb.putLong(0, 0);
bb.putLong(Long.BYTES, 0);
bb.putLong(2 * Long.BYTES, -1);

memTable.addEntry(ledgerId, 0, bb.duplicate(), this);

bb.putLong(2 * Long.BYTES, 0);

memTable.addEntry(ledgerId, 1, bb.duplicate(), this);

bb.putLong(2 * Long.BYTES, 1);

memTable.addEntry(ledgerId, 2, bb.duplicate(), this);

bb.putLong(2 * Long.BYTES, 2);

memTable.addEntry(ledgerId, 3, bb.duplicate(), this);

long lac = memTable.getLastAddConfirmed(ledgerId);

assertEquals(2, lac);
}
}

64 changes: 62 additions & 2 deletions ...eeper-server/src/test/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertThrows;

@codelipenghui codelipenghui Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkstyle (ImportOrder) fails here: assertThrows needs to sort between assertFalse and assertTrue. CI runs checkstyle:check (bk-ci.yml), so this will fail the build.


import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
Expand All @@ -46,6 +47,7 @@
import org.apache.bookkeeper.bookie.CheckpointSource.Checkpoint;
import org.apache.bookkeeper.conf.ServerConfiguration;
import org.apache.bookkeeper.conf.TestBKConfiguration;
import org.apache.bookkeeper.proto.BookieProtocol;

@codelipenghui codelipenghui Oct 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import (checkstyle UnusedImports): BookieProtocol isn't referenced anywhere in this test class.

import org.apache.bookkeeper.stats.NullStatsLogger;
import org.apache.bookkeeper.stats.StatsLogger;
import org.apache.bookkeeper.test.TestStatsProvider;
Expand Down Expand Up @@ -143,6 +145,11 @@ void checkEntry(long ledgerId, long entryId, long location) throws EntryLookupEx
final long entriesPerWrite = 2;
final long numOfLedgers = 5;

private void prepareLedger(final long ledgerId) throws IOException {
interleavedStorage.setMasterKey(ledgerId, ("ledger-" + ledgerId).getBytes());
interleavedStorage.setFenced(ledgerId);
}

@Before
public void setUp() throws Exception {
File tmpDir = File.createTempFile("bkTest", ".dir");
Expand All @@ -167,8 +174,7 @@ public void setUp() throws Exception {
for (long entryId = 0; entryId < numWrites; entryId++) {
for (long ledgerId = 0; ledgerId < numOfLedgers; ledgerId++) {
if (entryId == 0) {
interleavedStorage.setMasterKey(ledgerId, ("ledger-" + ledgerId).getBytes());
interleavedStorage.setFenced(ledgerId);
this.prepareLedger(ledgerId);
}
ByteBuf entry = Unpooled.buffer(128);
entry.writeLong(ledgerId);
Expand Down Expand Up @@ -449,4 +455,58 @@ void printInfoLine(String s) {
res = shell.run(new String[] { "localconsistencycheck" });
Assert.assertEquals(1, res);
}

@Test
public void testGetLastAddConfirmed() throws Exception {
final byte[] entry = new byte[Long.BYTES * 3];
final ByteBuf bb = Unpooled.wrappedBuffer(entry);

// Work off of a ledgerId that we haven't already inserted
final long ledgerId = this.numOfLedgers + 1;
final long entryId = 2;
final long lac = 1;

this.prepareLedger(ledgerId);
bb.setLong(0, ledgerId);
bb.setLong(Long.BYTES, entryId);
bb.setLong(2 * Long.BYTES, lac);

interleavedStorage.addEntry(bb);

final long lac1 = interleavedStorage.getLastAddConfirmed(ledgerId);
final long lac2 = interleavedStorage.getLastAddConfirmed(ledgerId);

interleavedStorage.removeLedgerCacheEntry(ledgerId);

final long lac3 = interleavedStorage.getLastAddConfirmed(ledgerId);

assertEquals(lac, lac1);
assertEquals(lac, lac2);
assertEquals(lac, lac3);
}

@Test
public void testGetLastAddConfirmedFromLedgerEntry() throws Exception {
final byte[] entry = new byte[Long.BYTES * 3];
final ByteBuf bb = Unpooled.wrappedBuffer(entry);

// Work off of a ledgerId that we haven't already inserted
final long ledgerId = this.numOfLedgers + 2;
final long entryId = 2;
final long lac = 1;

this.prepareLedger(ledgerId);
bb.setLong(0, ledgerId);
bb.setLong(Long.BYTES, entryId);
bb.setLong(2 * Long.BYTES, lac);

assertThrows("Entry 0 not found", Bookie.NoEntryException.class, () -> {
interleavedStorage.getLastAddConfirmedFromLedgerEntry(ledgerId);
});

interleavedStorage.addEntry(bb);

final long lac2 = interleavedStorage.getLastAddConfirmedFromLedgerEntry(ledgerId);
assertEquals(lac, lac2);
}
}
46 changes: 46 additions & 0 deletions bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.bookkeeper.bookie.CheckpointSource.Checkpoint;
import org.apache.bookkeeper.conf.ServerConfiguration;
import org.apache.bookkeeper.conf.TestBKConfiguration;
import org.apache.bookkeeper.proto.BookieProtocol;
import org.apache.bookkeeper.test.TestStatsProvider;
import org.apache.bookkeeper.util.DiskChecker;
import org.junit.Before;
Expand Down Expand Up @@ -191,4 +192,49 @@ public void testGetListOfEntriesOfLedgerAfterFlush() throws IOException {
}));
}
}

@Test
public void testLastAddConfirmedNotFlushedMissingInLedgerCache() throws Exception {
// Use a new ledgerId, so that prior tests don't interfere with this one
final long ledgerId = numOfLedgers + 1;
final byte[] writeKey = {0x00, 0x00, 0x00, 0x00};
final byte[] payload = new byte[3 * Long.BYTES];
ByteBuf bb = Unpooled.wrappedBuffer(payload);
HandleFactory hf = new HandleFactoryImpl(sortedLedgerStorage);
hf.getHandle(ledgerId, writeKey, false);

bb.setLong(0, ledgerId);

// Write first entry with invalid LAC
bb.setLong(Long.BYTES, 0);
bb.setLong(2 * Long.BYTES, BookieProtocol.INVALID_ENTRY_ID);
sortedLedgerStorage.addEntry(bb);

// LAC entry should be both in the memTable and ledger cache
final long lac1 = sortedLedgerStorage.getLastAddConfirmed(ledgerId);
assertEquals(BookieProtocol.INVALID_ENTRY_ID, lac1);

// But if the ledger cache is purged...
sortedLedgerStorage.removeLedgerCacheEntry(ledgerId);

// Then the memTable should still be consulted
final long lac2 = sortedLedgerStorage.getLastAddConfirmed(ledgerId);
assertEquals(BookieProtocol.INVALID_ENTRY_ID, lac2);

// With a new entry in sortedLedgerStorage...
bb.setLong(Long.BYTES, 1);
bb.setLong(2 * Long.BYTES, 0);
sortedLedgerStorage.addEntry(bb);

// We should get an updated LAC from the ledger cache
final long lac3 = sortedLedgerStorage.getLastAddConfirmed(ledgerId);
assertEquals(0, lac3);

// And once the ledger cache is purged again...
sortedLedgerStorage.removeLedgerCacheEntry(ledgerId);

// We should read from the memTable
final long lac4 = sortedLedgerStorage.getLastAddConfirmed(ledgerId);
assertEquals(0, lac4);
}
}