numinnex commented on code in PR #4025:
URL: https://github.com/apache/iggy/pull/4025#discussion_r3906533537
##########
core/partitions/src/poll_plan.rs:
##########
@@ -1032,6 +1034,54 @@ struct ChunkWalk {
corrupt: bool,
}
+/// A fragment sliced from a disk chunk keeps the WHOLE chunk allocation
+/// alive until the reply frame is written out, and a reply can sit in a
+/// per-connection mailbox for a while. Copy the matched bytes out when they
+/// cover less than this fraction of the chunk, so a sparse match (a
+/// `count=1` poll off a cold partition) cannot pin ~1 MiB per queued reply;
+/// a dense catch-up read keeps the zero-copy path, where retained bytes stay
+/// within this factor of shipped bytes.
+const SPARSE_CHUNK_PIN_DIVISOR: usize = 4;
Review Comment:
Disk tier verified fixed — the bound composes across chunks and segments,
the grow-and-re-read path is a provable no-op (`consumed == 0` means nothing
was pushed, so `borrowed == 0`), and `chunk.len()` is the right denominator
since `read_chunk_with_retry` never slices the buffer. Byte-for-byte parity
survives the in-place rewrite. Two things about the framing, though.
**The resident tier keeps the same pin.** `try_push_resident_entry`
(journal.rs, `Fragment::slice(prepare.clone(), ..)`) gets no unpinning. The
reason this was dismissed earlier — "the journal owns those bytes anyway" —
does not hold: `Journal::evict_prefix` (journal.rs:553) calls
`inner.storage.drain()`, which is `std::mem::take(entries)`
(journal.rs:282-291), on the routine commit-flush path. The journal releases
its refcount and a still-queued reply becomes sole owner of the whole prepare
allocation. That window is unbounded on single-replica groups, where the
`evicted_ring` backstop is gated off by `repair_retention`
(iggy_partition.rs:504-506).
This is introduced here, not pre-existing: on master
`build_polled_messages_body` consumed `PollFragments` by value and dropped
every refcount before the reply was enqueued, so no fragment outlived
`handle_poll_messages` on either tier.
Magnitude is workload-dependent and mostly benign — `batch_length` defaults
to 1000 on both producer and consumer, so the default path selects whole
batches, `borrowed` is approximately the whole prepare, and the predicate would
not copy anyway. The bad case is a small `count` against a large-batch
producer, where a partial selection ships ~1.3 KiB and pins the entire batch.
One caveat if you reuse this helper there: `execute_resident()` runs inline
on the shard pump (the disk path is detached through `spawn_poll_io`), so a
ratio-gated copy could put a ~1 MiB memcpy on the pump. An absolute byte floor
(copy when the selected bytes are under ~64 KiB, regardless of ratio) keeps the
copy bounded and fires only on the at-most-two partial batches per poll.
**The stated bound is not what the predicate delivers.** "retained bytes
stay within this factor of shipped bytes" (and "4x shipped bytes" in the commit
message) overstates it three ways: it is disk-tier only; `borrowed` excludes
the owned rewritten-header fragments that are also shipped, so the denominator
is not shipped bytes; and the copy allocates 4096-aligned, so a chunk
contributing 300 B still retains a page. Suggested replacement:
> On the disk tier, the chunk allocations a poll reply keeps alive are
bounded by `SPARSE_CHUNK_PIN_DIVISOR` times the record bytes it serves from
disk, plus one page per compacted chunk. This is a ratio, not an absolute cap —
a grown chunk can still retain tens of MiB — and it does not cover the resident
tier or the reply's absolute size.
--
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]
