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