You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "kalayciburak (via GitHub)" <gi...@apache.org> on 2026/10/04 07:35:46 UTC
[PR] Issue 4900: Read an unflushed ledger LAC from the memtable [bookkeeper]
kalayciburak opened a new pull request, #4901:
URL: https://github.com/apache/bookkeeper/pull/4901
Descriptions of the changes in this PR:
Fix #4900
### Motivation
`SortedLedgerStorage.getLastAddConfirmed` delegated entirely to the index. Until the memtable is flushed, that LAC lives only in a `FileInfo` cache entry. Once the cache evicts the ledger, the index file still does not exist, so a later read fails with `NoLedgerException` / `NoEntryException` even though the entry is still in the memtable. The bookie turns that into `ENOLEDGER`.
### Changes
On those misses, read the LAC from the last memtable entry (the same offset `addEntry` stores). If the entry was flushed between the miss and the lookup, ask the index again. A ledger that is in neither place still fails.
### Tests
`mvn -pl bookkeeper-server -am test -Dtest=SortedLedgerStorageTest -Dsurefire.failIfNoSpecifiedTests=false`
`SortedLedgerStorageTest` 6 tests, 0 failures. The new case expects LAC 3 and got `NoEntryException: Entry 0 not found in 0` before the change.
`mvn -pl bookkeeper-server checkstyle:check` — 0 violations.
--
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
Re: [PR] Issue 4900: Read an unflushed ledger LAC from the memtable [bookkeeper]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #4901:
URL: https://github.com/apache/bookkeeper/pull/4901#discussion_r4261985126
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageTest.java:
##########
@@ -191,4 +196,89 @@ public void testGetListOfEntriesOfLedgerAfterFlush() throws IOException {
}));
}
}
+
+ /**
+ * A newer memtable LAC must remain visible after an evicted FileInfo is flushed.
+ */
+ @Test
+ public void testGetLastAddConfirmedAfterFileInfoEviction() throws Exception {
+ File tmpDir = File.createTempFile("bkLac", ".dir");
+ tmpDir.delete();
+ tmpDir.mkdir();
+ BookieImpl.checkDirectoryStructure(BookieImpl.getCurrentDirectory(tmpDir));
+
+ ServerConfiguration limited = TestBKConfiguration.newServerConfiguration();
+ limited.setEntryLogPerLedgerEnabled(conf.isEntryLogPerLedgerEnabled());
+ limited.setLedgerDirNames(new String[] { tmpDir.toString() });
+ limited.setOpenFileLimit(2);
+ limited.setNumAddWorkerThreads(1);
+ limited.setNumReadWorkerThreads(1);
+ limited.setSkipListSizeLimit(64 * 1024 * 1024);
+
+ LedgerDirsManager dirs = new LedgerDirsManager(limited, limited.getLedgerDirs(),
+ new DiskChecker(limited.getDiskUsageThreshold(), limited.getDiskUsageWarnThreshold()));
+ InterleavedLedgerStorage underlying = new InterleavedLedgerStorage();
+ SortedLedgerStorage storage = new SortedLedgerStorage(underlying);
+ storage.initialize(limited, null, dirs, dirs,
+ statsProvider.getStatsLogger(BOOKIE_SCOPE), UnpooledByteBufAllocator.DEFAULT);
+ storage.setCheckpointSource(CheckpointSource.DEFAULT);
+ storage.setCheckpointer(checkpointer);
+ try {
+ long ledgerId = 0L;
+ storage.setMasterKey(ledgerId, ("ledger-" + ledgerId).getBytes());
+ for (long entryId = 0; entryId < 5; entryId++) {
+ ByteBuf entry = Unpooled.buffer(128);
+ entry.writeLong(ledgerId);
+ entry.writeLong(entryId);
+ entry.writeLong(entryId - 1);
+ entry.writeByte(1);
+ storage.addEntry(entry);
+ }
+ storage.flush();
Review Comment:
Since entries 0–4 are flushed here, the index lookup in `getLastAddConfirmed` succeeds (it returns 3), so this test only runs the try branch. The catch branch is the fix for #4900 itself: the index throws for an evicted ledger with nothing flushed. That branch isn't covered any more. The earlier version of this test was replaced, and `testGetLastAddConfirmedRetriesAfterIndexMiss` only reaches that branch with an empty memtable.
I checked locally with two changes to `SortedLedgerStorage#getLastAddConfirmed`. All 8 tests in this class still pass with either one:
- **The catch branch replaced by a plain index retry.** This brings #4900 back: an end-to-end V3 read of an evicted, unflushed ledger fails with `BKNoSuchEntryException`.
- **The catch branch returning the memtable LAC without the write-back.** A long poll after a LAC read then keeps waiting.
Removing the write-back in the try branch does fail this test, so it's a good guard for the partially flushed case.
Could you keep this test and add a fully unflushed case next to it? With the same setup:
1. Add entries 0–2 without flushing.
2. Evict with the same `invalidate` calls.
3. Assert LAC 1.
4. Assert `waitForLastAddConfirmedUpdate(ledgerId, 0, watcher)` returns `false` (the write-back reached the cache).
5. Call `flush()`, assert the memtable is empty, and assert LAC 1 again.
In my local checks, assertions like these fail under both changes above.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java:
##########
@@ -240,7 +240,46 @@ public ByteBuf getEntry(long ledgerId, long entryId) throws IOException, BookieE
@Override
public long getLastAddConfirmed(long ledgerId) throws IOException {
- return interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
+ Long memTableLac = lastAddConfirmedFromMemTable(ledgerId);
+ try {
+ long persistedLac = interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
+ if (memTableLac == null || memTableLac <= persistedLac) {
+ return persistedLac;
+ }
+ try {
+ return interleavedLedgerStorage.ledgerCache.updateLastAddConfirmed(ledgerId, memTableLac);
+ } catch (Bookie.NoLedgerException e) {
+ return memTableLac;
+ }
Review Comment:
Nit: a one-line comment on why the value is written back would help, for example: it keeps the cached LAC from falling back to the older index value once the memtable is flushed, and it notifies LAC watchers. The method returns the right value either way, so a later reader could take the write-back for dead code.
--
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
Re: [PR] Issue 4900: Read an unflushed ledger LAC from the memtable [bookkeeper]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #4901:
URL: https://github.com/apache/bookkeeper/pull/4901#discussion_r4262690779
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java:
##########
@@ -240,7 +240,57 @@ public ByteBuf getEntry(long ledgerId, long entryId) throws IOException, BookieE
@Override
public long getLastAddConfirmed(long ledgerId) throws IOException {
- return interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
+ LedgerCache ledgerCache = interleavedLedgerStorage.getLedgerCache();
+ Long cachedLac;
+ try {
+ cachedLac = ledgerCache.getLastAddConfirmed(ledgerId);
+ } catch (Bookie.NoLedgerException e) {
+ cachedLac = null;
+ }
+ if (cachedLac != null) {
+ return cachedLac;
+ }
+
+ Long memTableLac = lastAddConfirmedFromMemTable(ledgerId);
+ if (memTableLac != null) {
+ try {
+ return ledgerCache.updateLastAddConfirmed(ledgerId, memTableLac);
+ } catch (Bookie.NoLedgerException e) {
+ return memTableLac;
+ }
+ }
+
+ try {
+ return interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
+ } catch (Bookie.NoLedgerException | Bookie.NoEntryException e) {
+ // Recheck after the miss because the memtable can flush while the index is read.
Review Comment:
Nit: with the new order, this comment no longer describes the case. The index is only read after the memtable had nothing for this ledger, and unflushed entries stay visible in the memtable until all of them are in the index, so a flush can't cause this miss. The recheck now only matters if a first entry is added after the memtable check above. Maybe: `// An entry may have been added after the memtable check above.`
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageTest.java:
##########
@@ -191,4 +197,139 @@ public void testGetListOfEntriesOfLedgerAfterFlush() throws IOException {
}));
}
}
+
+ /**
+ * A newer memtable LAC must remain visible after an evicted FileInfo is flushed.
+ */
+ @Test
+ public void testGetLastAddConfirmedAfterFileInfoEviction() throws Exception {
+ File tmpDir = File.createTempFile("bkLac", ".dir");
+ tmpDir.delete();
+ tmpDir.mkdir();
+ BookieImpl.checkDirectoryStructure(BookieImpl.getCurrentDirectory(tmpDir));
+
+ ServerConfiguration limited = TestBKConfiguration.newServerConfiguration();
+ limited.setEntryLogPerLedgerEnabled(conf.isEntryLogPerLedgerEnabled());
+ limited.setLedgerDirNames(new String[] { tmpDir.toString() });
+ limited.setOpenFileLimit(2);
+ limited.setNumAddWorkerThreads(1);
+ limited.setNumReadWorkerThreads(1);
+ limited.setSkipListSizeLimit(64 * 1024 * 1024);
+
+ LedgerDirsManager dirs = new LedgerDirsManager(limited, limited.getLedgerDirs(),
+ new DiskChecker(limited.getDiskUsageThreshold(), limited.getDiskUsageWarnThreshold()));
+ InterleavedLedgerStorage underlying = new InterleavedLedgerStorage();
+ SortedLedgerStorage storage = new SortedLedgerStorage(underlying);
+ storage.initialize(limited, null, dirs, dirs,
+ statsProvider.getStatsLogger(BOOKIE_SCOPE), UnpooledByteBufAllocator.DEFAULT);
+ storage.setCheckpointSource(CheckpointSource.DEFAULT);
+ storage.setCheckpointer(checkpointer);
+ try {
+ long ledgerId = 0L;
+ storage.setMasterKey(ledgerId, ("ledger-" + ledgerId).getBytes());
+ for (long entryId = 0; entryId < 5; entryId++) {
+ ByteBuf entry = Unpooled.buffer(128);
+ entry.writeLong(ledgerId);
+ entry.writeLong(entryId);
+ entry.writeLong(entryId - 1);
+ entry.writeByte(1);
+ storage.addEntry(entry);
+ }
+ storage.flush();
+
+ for (long entryId = 5; entryId < 10; entryId++) {
+ ByteBuf entry = Unpooled.buffer(128);
+ entry.writeLong(ledgerId);
+ entry.writeLong(entryId);
+ entry.writeLong(entryId - 1);
+ entry.writeByte(1);
+ storage.addEntry(entry);
+ }
+ LedgerCacheImpl ledgerCache = (LedgerCacheImpl) underlying.ledgerCache;
+ IndexPersistenceMgr indexPersistenceManager = ledgerCache.getIndexPersistenceManager();
+ indexPersistenceManager.readFileInfoCache.invalidate(ledgerId);
+ indexPersistenceManager.writeFileInfoCache.invalidate(ledgerId);
+ indexPersistenceManager.readFileInfoCache.cleanUp();
+ indexPersistenceManager.writeFileInfoCache.cleanUp();
+
+ assertEquals("LAC of an evicted ledger must include newer memtable entries",
+ 8L, storage.getLastAddConfirmed(ledgerId));
+ storage.flush();
+ assertTrue("memtable must be empty after a real flush", storage.memTable.isEmpty());
+ assertEquals("LAC must not move backwards after the memtable is flushed",
+ 8L, storage.getLastAddConfirmed(ledgerId));
+ assertThrows(Bookie.NoLedgerException.class, () -> storage.getLastAddConfirmed(9_999L));
+ } finally {
+ storage.shutdown();
+ }
+ }
+
+ @Test
+ public void testGetLastAddConfirmedRetriesAfterIndexMiss() throws Exception {
+ InterleavedLedgerStorage underlying = mock(InterleavedLedgerStorage.class);
+ LedgerCache ledgerCache = mock(LedgerCache.class);
+ when(underlying.getLedgerCache()).thenReturn(ledgerCache);
+ when(ledgerCache.getLastAddConfirmed(0L)).thenReturn(null);
+ when(underlying.getLastAddConfirmed(0L))
+ .thenThrow(new Bookie.NoEntryException(0L, -1L))
+ .thenReturn(1L);
+ SortedLedgerStorage storage = new SortedLedgerStorage(underlying);
+ storage.initialize(conf, null, ledgerDirsManager, ledgerDirsManager,
+ statsProvider.getStatsLogger(BOOKIE_SCOPE), UnpooledByteBufAllocator.DEFAULT);
+ storage.setCheckpointSource(checkpointSource);
+ storage.setCheckpointer(checkpointer);
+ try {
+ assertEquals(1L, storage.getLastAddConfirmed(0L));
+ verify(underlying, times(2)).getLastAddConfirmed(0L);
+ } finally {
+ storage.shutdown();
+ }
+ }
+
+ @Test
+ public void testGetLastAddConfirmedUsesCachedValueBeforeMemTable() throws Exception {
+ InterleavedLedgerStorage underlying = mock(InterleavedLedgerStorage.class);
+ LedgerCache ledgerCache = mock(LedgerCache.class);
+ when(underlying.getLedgerCache()).thenReturn(ledgerCache);
+ when(ledgerCache.getLastAddConfirmed(0L)).thenReturn(8L);
+ SortedLedgerStorage storage = new SortedLedgerStorage(underlying);
+ storage.initialize(conf, null, ledgerDirsManager, ledgerDirsManager,
+ statsProvider.getStatsLogger(BOOKIE_SCOPE), UnpooledByteBufAllocator.DEFAULT);
+ storage.setCheckpointSource(checkpointSource);
+ storage.setCheckpointer(checkpointer);
+ EntryMemTable memTable = mock(EntryMemTable.class);
+ storage.memTable = memTable;
Review Comment:
Nit: `shutdown()` now closes the mock, so the real memtable created by `setCheckpointSource` is never closed. In the `entryLogPerLedger=true` run that's an `EntryMemTableWithParallelFlusher`, and its 8 flush threads keep running after the test. I counted 8 alive after `shutdown()`, and 0 without the swap.
The class already doesn't shut down its `setUp` storage, so this is optional. Either of these would avoid it:
- keep a reference to the original memtable and close it in `finally`;
- use `spy(storage.memTable)` with `verify(..., never()).getLastEntry(0L)`.
--
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
Re: [PR] Issue 4900: Read an unflushed ledger LAC from the memtable [bookkeeper]
Posted by "kalayciburak (via GitHub)" <gi...@apache.org>.
kalayciburak commented on PR #4901:
URL: https://github.com/apache/bookkeeper/pull/4901#issuecomment-6140327211
Thanks djsweet, fixed this in c46faa0. getLastAddConfirmed now checks the memtable before the index and keeps the higher LAC, while preserving the retry when the index misses during a flush. Added regression coverage for the existing-ledger case and the retry path.
--
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
Re: [PR] Issue 4900: Read an unflushed ledger LAC from the memtable [bookkeeper]
Posted by "djsweet (via GitHub)" <gi...@apache.org>.
djsweet commented on PR #4901:
URL: https://github.com/apache/bookkeeper/pull/4901#issuecomment-5984800068
Thinking a bit more on this, I'm not sure this is a fully correct fix. Consider the scenario posed by #4900, but where the first ledger already exists, and is committed into InterleavedLedgerStorage.
If you write to over 20,000 ledgers with small payloads below the `skipListSizeLimit`, then the call to `interleavedLedgerStorage.getLastAddConfirmed` will not be able to access the LAC from the Ledger Cache, but instead will read from the committed value in `InterleavedLedgerStorage`, which will be behind the LAC in the `memTable`. This means the LAC gets reset backwards, spuriously.
#4902 always reads from the `entryMemTable` before `interleavedLedgerStorage`, so it shouldn't be susceptible to the same problem.
--
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
Re: [PR] Issue 4900: Read an unflushed ledger LAC from the memtable [bookkeeper]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #4901:
URL: https://github.com/apache/bookkeeper/pull/4901#discussion_r4333904153
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java:
##########
@@ -240,7 +240,75 @@ public ByteBuf getEntry(long ledgerId, long entryId) throws IOException, BookieE
@Override
public long getLastAddConfirmed(long ledgerId) throws IOException {
- return interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
+ LedgerCache ledgerCache = interleavedLedgerStorage.getLedgerCache();
+ Long cachedLac;
+ try {
+ cachedLac = ledgerCache.getLastAddConfirmed(ledgerId);
+ } catch (Bookie.NoLedgerException e) {
+ if (!interleavedLedgerStorage.ledgerExists(ledgerId)) {
+ throw e;
+ }
+ cachedLac = null;
Review Comment:
Simpler: don't catch `NoLedgerException` here, and drop all six `ledgerExists` checks:
```java
LedgerCache ledgerCache = interleavedLedgerStorage.getLedgerCache();
// Throws NoLedgerException for unknown or deleted ledgers. An evicted FileInfo
// reloads from its index file without a LAC, so this returns null instead.
Long cachedLac = ledgerCache.getLastAddConfirmed(ledgerId);
if (cachedLac != null) {
return cachedLac;
}
// memtable step and index fallback as they are, minus the ledgerExists checks
```
That keeps the behavior you want:
- **#4900 (evicted, nothing flushed):** eviction closes the FileInfo with `close(true)`, which writes a header-only index file. The reload finds that file and gets a FileInfo without a LAC, so this lookup returns `null` instead of throwing. The catch is never reached on this path.
- **Deleted or unknown ledger:** the lookup throws `NoLedgerException`, because the index file is gone or the FileInfo is marked deleted. A V3 LAC read then gets ENOLEDGER, as on master.
- The remaining checks only cover a deletion racing this call, which no check can close.
The only other case I found where this lookup throws `NoLedgerException` for a live ledger is a failed index relocation, which leaves the ledger's index broken anyway.
Locally on 5c77e65 (same PR files), this version passed your real-storage tests, including `testGetLastAddConfirmedDoesNotReadDeletedLedgerFromMemTable`, plus an end-to-end V3 read of an evicted, unflushed ledger and a deleted-ledger check with the FileInfo evicted first. Only two mock tests failed:
- `testGetLastAddConfirmedRejectsCachedValueForDeletedLedger` mocks a cached LAC for a deleted ledger, which real storage doesn't produce. It can go.
- `testGetLastAddConfirmedFallsBackToMemTableWhenCacheMisses` models eviction as the cache throwing. A real eviction returns `null`, so `thenReturn(null)` matches it.
The `ledgerExists` stubs would then be unused. Sorry for the churn: this catch came from my earlier snippet, and it wasn't needed.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java:
##########
@@ -240,7 +240,75 @@ public ByteBuf getEntry(long ledgerId, long entryId) throws IOException, BookieE
@Override
public long getLastAddConfirmed(long ledgerId) throws IOException {
- return interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
+ LedgerCache ledgerCache = interleavedLedgerStorage.getLedgerCache();
+ Long cachedLac;
+ try {
+ cachedLac = ledgerCache.getLastAddConfirmed(ledgerId);
+ } catch (Bookie.NoLedgerException e) {
+ if (!interleavedLedgerStorage.ledgerExists(ledgerId)) {
+ throw e;
+ }
+ cachedLac = null;
+ }
+ if (cachedLac != null) {
+ if (!interleavedLedgerStorage.ledgerExists(ledgerId)) {
+ throw new Bookie.NoLedgerException(ledgerId);
+ }
Review Comment:
This adds `ledgerExists()` to every cached LAC read, which is the path every successful V3 entry read takes (`ReadEntryProcessorV3` reads the LAC after each entry). It ends in `SnapshotMap.containsKey`, which takes a read lock shared by the whole storage and does three map lookups.
In a local micro-benchmark (1000 ledgers, LAC cached), 8-thread throughput on this path dropped from ~6.2 to ~3.75 M calls/s, and single-thread cost went from ~34 to ~40 ns. A tight loop overstates the end-to-end effect, but this is the path f1aef21 had just brought back to master's cost.
A cached LAC for a deleted ledger only exists while `deleteLedger` is running. `removeLedger` deletes the index file and invalidates both file-info caches, and any reload after that throws `NoLedgerException`. The window between `activeLedgers.remove` and the FileInfo deletion exists on master too, and a check here can't close it, since the deletion can start right after the check.
Could this check be dropped? The comment above suggests a version without any of these checks.
--
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
Re: [PR] Issue 4900: Read an unflushed ledger LAC from the memtable [bookkeeper]
Posted by "djsweet (via GitHub)" <gi...@apache.org>.
djsweet commented on PR #4901:
URL: https://github.com/apache/bookkeeper/pull/4901#issuecomment-5981638470
I have an alternative PR, #4902, that we've been using in production as a workaround for this bug.
It's worth pointing out that both of these PRs require a fix for #4895 to function correctly when ledger metadata is in use.
--
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
Re: [PR] Issue 4900: Read an unflushed ledger LAC from the memtable [bookkeeper]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #4901:
URL: https://github.com/apache/bookkeeper/pull/4901#discussion_r4260813570
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java:
##########
@@ -240,7 +240,39 @@ public ByteBuf getEntry(long ledgerId, long entryId) throws IOException, BookieE
@Override
public long getLastAddConfirmed(long ledgerId) throws IOException {
- return interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
+ // The index can still expose the committed LAC while a newer entry is in the memtable.
+ Long memTableLac = lastAddConfirmedFromMemTable(ledgerId);
+ try {
+ long persistedLac = interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
+ if (memTableLac == null) {
+ return persistedLac;
+ }
+ return Math.max(persistedLac, memTableLac);
Review Comment:
The returned value is now correct while the newer entries are still in the memtable. But `interleavedLedgerStorage.getLastAddConfirmed()` still caches the index-derived LAC in the reloaded `FileInfo` (`InterleavedLedgerStorage#getLastAddConfirmed` calls `ledgerCache.updateLastAddConfirmed`). Nothing writes the memtable value back.
Repro, using a real `SortedLedgerStorage` with `CheckpointSource.DEFAULT` so `flush()` really flushes:
1. Add entries 0–4 (LAC = entryId − 1) and flush. Add entries 5–9. Evict ledger 0's FileInfo.
2. `getLastAddConfirmed(0)` returns 8, which is correct. But the ledger cache now holds 3.
3. Call `flush()` while the FileInfo is still cached. `getLastAddConfirmed(0)` now returns **3**.
So once the rest of the ledger is flushed, the LAC still goes backwards (8 → 3), which is the case @djsweet described. It stays at 3 until a newer LAC arrives or the FileInfo is evicted again.
Writing the winning value back fixes it. `FileInfo#setLastAddConfirmed` only moves forward and also notifies long-poll watchers:
```java
if (memTableLac == null || memTableLac <= persistedLac) {
return persistedLac;
}
return interleavedLedgerStorage.ledgerCache.updateLastAddConfirmed(ledgerId, memTableLac);
```
The catch branch needs the same write-back for `latestMemTableLac`, ignoring `NoLedgerException`. With that, the repro returns 8 both before and after the flush. Alternatively, check the cached LAC first (see the next comment).
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java:
##########
@@ -240,7 +240,39 @@ public ByteBuf getEntry(long ledgerId, long entryId) throws IOException, BookieE
@Override
public long getLastAddConfirmed(long ledgerId) throws IOException {
- return interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
+ // The index can still expose the committed LAC while a newer entry is in the memtable.
+ Long memTableLac = lastAddConfirmedFromMemTable(ledgerId);
Review Comment:
This puts a memtable lookup in front of every call: a read lock, up to two skip-list `floorKey` calls, and a get-entry stats event in `EntryMemTable#getLastEntry`. That includes the common case where the FileInfo already has the LAC cached, and `ReadEntryProcessorV3` calls this on every V3 entry read.
I ran a quick local loop: 1000 ledgers × 200 unflushed entries, LAC cached.
- Single thread: about 38 ns per call on master, about 160 ns with this change.
- 8 threads: about 8.5 M calls/s on master, about 3.9 M with this change.
Every LAC read also counts as a memtable get in the skip-list stats.
Checking the cache first keeps the common path the same as master and only touches the memtable after an eviction. This is essentially the order #4902 uses:
```java
LedgerCache ledgerCache = interleavedLedgerStorage.ledgerCache;
Long lac = null;
try {
lac = ledgerCache.getLastAddConfirmed(ledgerId);
} catch (Bookie.NoLedgerException nle) {
// no FileInfo/index; the memtable may still have the ledger
}
if (lac != null) {
return lac;
}
Long memLac = lastAddConfirmedFromMemTable(ledgerId);
if (memLac != null) {
try {
return ledgerCache.updateLastAddConfirmed(ledgerId, memLac);
} catch (Bookie.NoLedgerException nle) {
return memLac;
}
}
// nothing unflushed (or it was just flushed): the index is complete
return interleavedLedgerStorage.getLastAddConfirmed(ledgerId);
```
This is safe against a concurrent flush. `flushSnapshot` writes every snapshot entry to the index before `clearSnapshot`, and `getLastEntry` still sees the snapshot until then. So if the memtable has nothing, the index already has everything, and the retry is no longer needed.
Locally this passes the eviction test, the partially flushed repro above and an end-to-end V3 read, at the same per-call cost as master.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageTest.java:
##########
@@ -191,4 +196,101 @@ public void testGetListOfEntriesOfLedgerAfterFlush() throws IOException {
}));
}
}
+
+ /**
+ * An unflushed ledger's LAC lives in a FileInfo cache entry. Once that entry is
+ * evicted, the index file still does not exist, so the LAC has to come from the memtable.
+ */
+ @Test
+ public void testGetLastAddConfirmedAfterFileInfoEviction() throws Exception {
+ File tmpDir = File.createTempFile("bkLac", ".dir");
+ tmpDir.delete();
+ tmpDir.mkdir();
+ BookieImpl.checkDirectoryStructure(BookieImpl.getCurrentDirectory(tmpDir));
+
+ ServerConfiguration limited = TestBKConfiguration.newServerConfiguration();
+ limited.setEntryLogPerLedgerEnabled(conf.isEntryLogPerLedgerEnabled());
+ limited.setLedgerDirNames(new String[] { tmpDir.toString() });
+ limited.setOpenFileLimit(2);
+ limited.setNumAddWorkerThreads(1);
+ limited.setNumReadWorkerThreads(1);
+ limited.setSkipListSizeLimit(64 * 1024 * 1024);
+
+ LedgerDirsManager dirs = new LedgerDirsManager(limited, limited.getLedgerDirs(),
+ new DiskChecker(limited.getDiskUsageThreshold(), limited.getDiskUsageWarnThreshold()));
+ SortedLedgerStorage storage = new SortedLedgerStorage();
+ storage.initialize(limited, null, dirs, dirs,
+ statsProvider.getStatsLogger(BOOKIE_SCOPE), UnpooledByteBufAllocator.DEFAULT);
+ storage.setCheckpointSource(checkpointSource);
+ storage.setCheckpointer(checkpointer);
+ try {
+ long lac = 3L;
+ int ledgers = 12;
+ for (long ledgerId = 0; ledgerId < ledgers; ledgerId++) {
+ storage.setMasterKey(ledgerId, ("ledger-" + ledgerId).getBytes());
+ ByteBuf entry = Unpooled.buffer(128);
+ entry.writeLong(ledgerId);
+ entry.writeLong(0L);
+ entry.writeLong(lac);
+ entry.writeByte(1);
+ storage.addEntry(entry);
+ }
+ assertEquals("LAC of an unflushed ledger after file-info eviction",
+ lac, storage.getLastAddConfirmed(0L));
+ storage.flush();
Review Comment:
This `flush()` doesn't flush the memtable. In this class, `checkpointSource.newCheckpoint()` returns `Checkpoint.MAX`. `EntryMemTable#snapshot(Checkpoint.MAX)` only takes a snapshot when `kvmap.cp.compareTo(MAX) < 0`, which is never true for `MAX`.
I checked that entry 0 is still in `storage.memTable` afterwards. So the next assertion runs the memtable path again, not the post-flush index path.
Using `CheckpointSource.DEFAULT` (which returns `Checkpoint.MIN`) for this storage, and asserting `storage.memTable.isEmpty()` after the flush, would make it test what the message says. The current code does return the right LAC from the index after a real flush.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageTest.java:
##########
@@ -191,4 +196,101 @@ public void testGetListOfEntriesOfLedgerAfterFlush() throws IOException {
}));
}
}
+
+ /**
+ * An unflushed ledger's LAC lives in a FileInfo cache entry. Once that entry is
+ * evicted, the index file still does not exist, so the LAC has to come from the memtable.
+ */
+ @Test
+ public void testGetLastAddConfirmedAfterFileInfoEviction() throws Exception {
+ File tmpDir = File.createTempFile("bkLac", ".dir");
+ tmpDir.delete();
+ tmpDir.mkdir();
+ BookieImpl.checkDirectoryStructure(BookieImpl.getCurrentDirectory(tmpDir));
+
+ ServerConfiguration limited = TestBKConfiguration.newServerConfiguration();
+ limited.setEntryLogPerLedgerEnabled(conf.isEntryLogPerLedgerEnabled());
+ limited.setLedgerDirNames(new String[] { tmpDir.toString() });
+ limited.setOpenFileLimit(2);
+ limited.setNumAddWorkerThreads(1);
+ limited.setNumReadWorkerThreads(1);
+ limited.setSkipListSizeLimit(64 * 1024 * 1024);
+
+ LedgerDirsManager dirs = new LedgerDirsManager(limited, limited.getLedgerDirs(),
+ new DiskChecker(limited.getDiskUsageThreshold(), limited.getDiskUsageWarnThreshold()));
+ SortedLedgerStorage storage = new SortedLedgerStorage();
+ storage.initialize(limited, null, dirs, dirs,
+ statsProvider.getStatsLogger(BOOKIE_SCOPE), UnpooledByteBufAllocator.DEFAULT);
+ storage.setCheckpointSource(checkpointSource);
+ storage.setCheckpointer(checkpointer);
+ try {
+ long lac = 3L;
+ int ledgers = 12;
+ for (long ledgerId = 0; ledgerId < ledgers; ledgerId++) {
+ storage.setMasterKey(ledgerId, ("ledger-" + ledgerId).getBytes());
+ ByteBuf entry = Unpooled.buffer(128);
+ entry.writeLong(ledgerId);
+ entry.writeLong(0L);
+ entry.writeLong(lac);
+ entry.writeByte(1);
+ storage.addEntry(entry);
+ }
+ assertEquals("LAC of an unflushed ledger after file-info eviction",
+ lac, storage.getLastAddConfirmed(0L));
+ storage.flush();
+ assertEquals("LAC still readable after the memtable is flushed",
+ lac, storage.getLastAddConfirmed(0L));
+ try {
+ storage.getLastAddConfirmed(9_999L);
+ assertTrue("missing ledger should not report a LAC", false);
+ } catch (Bookie.NoLedgerException | Bookie.NoEntryException expected) {
+ // index has no file and the memtable has no entry
+ }
Review Comment:
Nit: `assertThrows(IOException.class, () -> storage.getLastAddConfirmed(9_999L))` reads better than `assertTrue(msg, false)`.
Also, entry 0 can't carry LAC 3 (`lac = 3L` above), because a piggybacked LAC is always lower than the entry id. Writing a few entries with `LAC = entryId - 1` would keep the test realistic.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageTest.java:
##########
@@ -191,4 +196,101 @@ public void testGetListOfEntriesOfLedgerAfterFlush() throws IOException {
}));
}
}
+
+ /**
+ * An unflushed ledger's LAC lives in a FileInfo cache entry. Once that entry is
+ * evicted, the index file still does not exist, so the LAC has to come from the memtable.
+ */
+ @Test
+ public void testGetLastAddConfirmedAfterFileInfoEviction() throws Exception {
+ File tmpDir = File.createTempFile("bkLac", ".dir");
+ tmpDir.delete();
+ tmpDir.mkdir();
+ BookieImpl.checkDirectoryStructure(BookieImpl.getCurrentDirectory(tmpDir));
+
+ ServerConfiguration limited = TestBKConfiguration.newServerConfiguration();
+ limited.setEntryLogPerLedgerEnabled(conf.isEntryLogPerLedgerEnabled());
+ limited.setLedgerDirNames(new String[] { tmpDir.toString() });
+ limited.setOpenFileLimit(2);
+ limited.setNumAddWorkerThreads(1);
+ limited.setNumReadWorkerThreads(1);
+ limited.setSkipListSizeLimit(64 * 1024 * 1024);
+
+ LedgerDirsManager dirs = new LedgerDirsManager(limited, limited.getLedgerDirs(),
+ new DiskChecker(limited.getDiskUsageThreshold(), limited.getDiskUsageWarnThreshold()));
+ SortedLedgerStorage storage = new SortedLedgerStorage();
+ storage.initialize(limited, null, dirs, dirs,
+ statsProvider.getStatsLogger(BOOKIE_SCOPE), UnpooledByteBufAllocator.DEFAULT);
+ storage.setCheckpointSource(checkpointSource);
+ storage.setCheckpointer(checkpointer);
+ try {
+ long lac = 3L;
+ int ledgers = 12;
+ for (long ledgerId = 0; ledgerId < ledgers; ledgerId++) {
+ storage.setMasterKey(ledgerId, ("ledger-" + ledgerId).getBytes());
+ ByteBuf entry = Unpooled.buffer(128);
+ entry.writeLong(ledgerId);
+ entry.writeLong(0L);
+ entry.writeLong(lac);
+ entry.writeByte(1);
+ storage.addEntry(entry);
+ }
+ assertEquals("LAC of an unflushed ledger after file-info eviction",
+ lac, storage.getLastAddConfirmed(0L));
+ storage.flush();
+ assertEquals("LAC still readable after the memtable is flushed",
+ lac, storage.getLastAddConfirmed(0L));
+ try {
+ storage.getLastAddConfirmed(9_999L);
+ assertTrue("missing ledger should not report a LAC", false);
+ } catch (Bookie.NoLedgerException | Bookie.NoEntryException expected) {
+ // index has no file and the memtable has no entry
+ }
+ } finally {
+ storage.shutdown();
+ }
+ }
+
+ /**
+ * A committed LAC can be stale while a newer entry is still in the memtable.
+ */
+ @Test
+ public void testGetLastAddConfirmedPrefersMemTableForExistingLedger() throws Exception {
+ InterleavedLedgerStorage underlying = mock(InterleavedLedgerStorage.class);
+ when(underlying.getLastAddConfirmed(0L)).thenReturn(0L);
Review Comment:
Mocking `InterleavedLedgerStorage` hides the side effect behind the remaining bug. The real `getLastAddConfirmed()` also stores the index-derived LAC in the FileInfo cache, and that is what later comes back as the stale value. So this test passes while a real storage still goes 8 → 3 (see the comment on `SortedLedgerStorage`). Both mock tests would also need rework with either fix, because a mock's `ledgerCache` field is `null`.
Could this be a real-storage test instead? Roughly:
1. Use `CheckpointSource.DEFAULT` so `flush()` really flushes.
2. Add entries 0–4 and flush, then add 5–9.
3. Evict ledger 0's FileInfo, either with `openFileLimit` pressure as in the test above or by invalidating both `IndexPersistenceMgr` caches from the test package.
4. Assert 8, call `flush()`, and assert 8 again.
The last assertion fails on the current head.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageTest.java:
##########
@@ -191,4 +196,101 @@ public void testGetListOfEntriesOfLedgerAfterFlush() throws IOException {
}));
}
}
+
+ /**
+ * An unflushed ledger's LAC lives in a FileInfo cache entry. Once that entry is
+ * evicted, the index file still does not exist, so the LAC has to come from the memtable.
Review Comment:
Nit: the index file does exist after eviction. `FileInfoBackingCache#releaseFileInfo` calls `FileInfo.close(true)`, which creates a header-only `.idx` (1024 bytes). What's missing are the index entries. That's why the lookup throws `NoEntryException`, and why master returns `ENOENTRY` here rather than `ENOLEDGER`.
--
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