codelipenghui commented on code in PR #4902:
URL: https://github.com/apache/bookkeeper/pull/4902#discussion_r4260744807
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorageTest.java:
##########
@@ -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;
Review Comment:
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.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java:
##########
@@ -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) {
Review Comment:
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.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryMemTable.java:
##########
@@ -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) {
Review Comment:
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)`.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java:
##########
@@ -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);
+ 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);
Review Comment:
`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`.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorageTest.java:
##########
@@ -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;
Review Comment:
Unused import (checkstyle `UnusedImports`): `BookieProtocol` isn't
referenced anywhere in this test class.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java:
##########
@@ -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);
Review Comment:
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.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryMemTableTest.java:
##########
@@ -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 {
Review Comment:
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.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java:
##########
@@ -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);
+ 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);
+ }
+ }
+ if (null == lac) {
+ lac =
interleavedLedgerStorage.getLastAddConfirmedFromLedgerEntry(ledgerId);
+ }
+ return lac;
Review Comment:
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.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java:
##########
@@ -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 {
Review Comment:
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.
--
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]
