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