merlimat commented on PR #4741:
URL: https://github.com/apache/bookkeeper/pull/4741#issuecomment-4435263106
@dao-jun Thanks for the optimization. I went through the PR end-to-end —
overall the direction is good and the framing-budget contract is clean and
consistent across the storage stack. A few things I'd like to discuss before
merge.
## Overall
The core idea is sound: today the batch-read path reads each entry from disk
and only then checks whether it fits in `maxSize`, throwing the result away on
overflow. This PR threads the remaining response budget down through `Bookie →
LedgerDescriptor → LedgerStorage → EntryLogger`, lets
`DefaultEntryLogger`/`DirectEntryLogger` short-circuit by reading just the size
header, and updates `BatchedReadEntryProcessor` to use the bounded API for
entries 2..N (keeping the "always return the first entry" semantics).
The framing budget contract is consistent end-to-end (`entry.readableBytes()
+ 4 > maxEntrySize` everywhere, matching what `BatchedReadEntryProcessor` adds
to `frameSize`). Tests are thorough — exact-fit, off-by-one budgets,
oversized-first-entry, missing entries, IOException propagation, all the
storage flavors. Reference counting on the new paths checks out:
`WriteCache.get()`/`ReadCache.get()` return freshly-allocated buffers,
`readEntrySize` returns a thread-local, and `ByteBufList.deallocate` releases
members.
## Substantive concerns
**1. Undocumented behavior change in exception handling —
`BatchedReadEntryProcessor.readData`**
```java
} catch (Bookie.NoEntryException e) {
if (data == null) { throw e; }
break;
} catch (Throwable e) {
if (data != null) { data.release(); }
throw e;
}
```
The old code's `catch (Throwable)` silently broke and returned partial data
on *any* error after the first entry — including transient `IOException`s. The
new code only swallows `NoEntryException`; any other exception now propagates
as `EIO` to the client, even if some entries were already read successfully.
This is arguably more correct (hiding I/O errors is bad), but it's a real
behavior change that isn't called out in the description and may surprise
clients that relied on the lenient behavior. Could you call this out explicitly
in the PR description?
**2. Stat pollution in `BookieImpl.readEntryIfFits`**
When `entry == null` (size-rejected), `entrySize` is 0, but the method still
calls `registerSuccessfulValue(0)` on `readBytesStats` and records latency on
`readEntryStats`. That mixes "actually read N bytes" and "size-rejected, didn't
read" into the same histograms — the bytes histogram gets zero-spikes, and the
latency histogram skews artificially low because size-rejected reads are much
faster than full reads. Either skip recording on the null branch, or add a
separate counter for size-rejected reads.
**3. Significant code duplication in
`SingleDirectoryDbLedgerStorage.doGetEntryIfFits`**
`doGetEntryIfFits` is ~90 lines that mostly mirror `doGetEntry`, with the
same `if (entry.readableBytes() + Integer.BYTES > maxEntrySize) { release;
return null; }` block repeated four times (write cache,
write-cache-being-flushed, read cache, after entry-log read). Could be reduced
significantly by extracting a small `checkBudget(ByteBuf, long)` helper, or by
adding a budget parameter to `doGetEntry` with a sentinel for "no limit".
Smaller diff, easier to keep in sync going forward.
**4. `MockBookies.batchReadEntries` no longer releases the skipped entry**
The old code did `entry.release()` before breaking on overflow; the new code
drops that call. This is actually a fix — `MockLedgerData.getEntry()` returns
the same shared `ByteBuf` it stores in its map, so the old `release()` could
free the only ref and corrupt subsequent reads (which is exactly what
`testBatchReadDoesNotReleaseOversizedSkippedEntry` verifies). But the
production `BatchedReadEntryProcessor` *does* release in the analogous overflow
path (because `Bookie.readEntry` returns a freshly-allocated buffer), so the
mock and the real bookie now have intentionally different ref-counting
contracts. A short comment on the mock explaining "we don't own the buffer
here" would prevent future confusion.
## Smaller things
**5.** Inconsistent `+ 4` vs `+ Integer.BYTES` — `BatchedReadEntryProcessor`
uses literal `4`, every other module uses `Integer.BYTES`. Pick one.
**6.** `DefaultEntryLogger.readEntryIfFits` returns null on oversized
entries *before* calling `validateEntry`. If an entry's size header is
corrupted to a huge value, this silently returns null ("doesn't fit") instead
of catching the corruption. Probably acceptable — a later legitimate read would
catch it — but worth a one-line comment so a future reader doesn't think it's
an oversight.
**7.** When `entryLogger.readEntryIfFits` returns null in
`SingleDirectoryDbLedgerStorage.doGetEntryIfFits`, the entry isn't put in the
read cache. A subsequent call with a larger budget will hit disk again.
Probably fine (we never actually read the data), just noting it.
## Verdict
Direction is good, contract is clean, tests are solid. Before merging I'd
ask for: (a) the exception-handling behavior change explicitly called out in
the PR description, (b) stat recording tightened for size-rejected reads, and
(c) the `doGetEntryIfFits` duplication reduced. The rest are nice-to-have.
--
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]
