hubcio commented on code in PR #3786:
URL: https://github.com/apache/iggy/pull/3786#discussion_r3706643859
##########
core/shard/src/lib.rs:
##########
@@ -1841,23 +2391,98 @@ where
if partitions.contains(&namespace) {
return ParkOutcome::Deliver(message);
}
+ // Read the committed revision before taking the borrow below: the
frame
+ // is stamped with the incarnation it was addressed to, so a later
drain
+ // can tell it apart from a same-key replacement.
+ let epoch = self
+ .plane
+ .metadata()
+ .mux_stm
+ .streams()
+ .created_revision_for_namespace(namespace);
+ let frame_cost = parked_footprint(message.as_slice().len());
+ let replicated = message.header().command() != Command2::Request;
let mut pending = self.pending_partition_frames.borrow_mut();
- let parked = pending.entry(namespace).or_default();
- if parked.len() >= MAX_PARKED_PER_NAMESPACE {
- tracing::warn!(
- shard = self.id,
- namespace_raw = namespace.inner(),
- "parked-frame buffer full; dropping partition frame"
+ let parked_bytes = self.parked_partition_bytes.get();
+ // Read the entry without `entry().or_default()`: inserting first would
+ // leave an empty entry behind on the overflow path below, which reads
as
+ // a parked namespace to the reconciler sweep and its fast-skip guard.
+ let existing = pending.get_mut(&namespace);
+ let parked_len = existing.as_ref().map_or(0, |entry|
entry.frames.len());
+ let namespace_bytes = existing.as_ref().map_or(0, |entry| entry.bytes);
+ // A prepare is never shed on a byte budget. No client to answer, and
no
+ // recovery: `consensus::retransmit_targets` skips an op that already
+ // reached quorum and the plane opens a repair session only in
+ // `on_start_view`, so shedding one is permanent loss where shedding a
+ // request costs a retry. A request is refused the moment admitting it
+ // would cross a budget; a prepare only once one is already spent. Caps
+ // prepare residency at one frame of overshoot per budget (worst case
+ // `MAX_PARKED_BYTES` + `max_message_size`, 80 MiB per shard) instead
of
+ // at the budget, and is what makes an oversize frame parkable at all.
+ let namespace_budget_spent = parked_len > 0
+ && if replicated {
+ namespace_bytes >= MAX_PARKED_BYTES_PER_NAMESPACE
+ } else {
+ namespace_bytes.saturating_add(frame_cost) >
MAX_PARKED_BYTES_PER_NAMESPACE
+ };
+ let shard_budget_spent = if replicated {
+ parked_bytes >= MAX_PARKED_BYTES
+ } else {
+ parked_bytes.saturating_add(frame_cost) > MAX_PARKED_BYTES
+ };
+ if parked_len >= MAX_PARKED_PER_NAMESPACE || namespace_budget_spent ||
shard_budget_spent {
Review Comment:
the frame cap still sheds prepares: `parked_len >= MAX_PARKED_PER_NAMESPACE`
is checked unconditionally, before the class-split budget checks get a say. for
footprints under 32 KiB (4 MiB / 128) the 128-frame cap fires long before the
per-namespace byte budget ever could - reproduced: 128 header-only prepares
park, the 129th is shed with `park_overflow` bumped, at 512 KiB of a 4 MiB
budget. the byte budgets never fire. so small-append prepares are destroyed
exactly as before this fix, and the backoff change widens the window: prepares
are now retained across a failing build (up to 60s), and 128 small prepares in
60s is 2/sec. reachable at every replica count, not just 2 - quorum always has
at least one surplus backup (n=2 primary self-acks alone, n=3 quorum is 2 of
3), so one lagging backup loses shed prepares with nobody noticing.
this also contradicts the new docs in four places ("byte budgets bound those
instead", the three-axes paragraph, "a shed frame must never grow it past
that"), and the module doc's known-gaps TODO says exposure is narrowed to two
cases - the frame cap is an undocumented third.
fix is the same shape as the budget split: `parked_len >=
MAX_PARKED_PER_NAMESPACE && !replicated`. verified against this tree: prepares
then admit to exactly 1024 per namespace (4 MiB) before the byte budget sheds,
requests still cap at 128, all existing tests stay green, and every drain path
still works (re-dispatch, discard, age, stale-reject; a backup's replicate path
never touches the primary-side prepare queue, so no assert hazard). two things
must land with it: the four doc sites above flip stale the moment the cap
becomes request-only, and a test is mandatory - the one-liner breaks no
existing test, so it would land uncovered. shape: >128 small prepares retained
with the byte budget as the shedder, plus a request into the same deep entry
still shed at 128. worth knowing: with 1024 parkable prepares one namespace's
re-dispatch can now fill the default 1024-slot inbox where it was 1/8th - the
first-`Full` re-park handles it, other namespaces just converge a bit later.
##########
core/shard/src/lib.rs:
##########
@@ -1841,23 +2391,98 @@ where
if partitions.contains(&namespace) {
return ParkOutcome::Deliver(message);
}
+ // Read the committed revision before taking the borrow below: the
frame
+ // is stamped with the incarnation it was addressed to, so a later
drain
+ // can tell it apart from a same-key replacement.
+ let epoch = self
+ .plane
+ .metadata()
+ .mux_stm
+ .streams()
+ .created_revision_for_namespace(namespace);
+ let frame_cost = parked_footprint(message.as_slice().len());
+ let replicated = message.header().command() != Command2::Request;
let mut pending = self.pending_partition_frames.borrow_mut();
- let parked = pending.entry(namespace).or_default();
- if parked.len() >= MAX_PARKED_PER_NAMESPACE {
- tracing::warn!(
- shard = self.id,
- namespace_raw = namespace.inner(),
- "parked-frame buffer full; dropping partition frame"
+ let parked_bytes = self.parked_partition_bytes.get();
+ // Read the entry without `entry().or_default()`: inserting first would
+ // leave an empty entry behind on the overflow path below, which reads
as
+ // a parked namespace to the reconciler sweep and its fast-skip guard.
+ let existing = pending.get_mut(&namespace);
+ let parked_len = existing.as_ref().map_or(0, |entry|
entry.frames.len());
+ let namespace_bytes = existing.as_ref().map_or(0, |entry| entry.bytes);
+ // A prepare is never shed on a byte budget. No client to answer, and
no
+ // recovery: `consensus::retransmit_targets` skips an op that already
+ // reached quorum and the plane opens a repair session only in
+ // `on_start_view`, so shedding one is permanent loss where shedding a
+ // request costs a retry. A request is refused the moment admitting it
+ // would cross a budget; a prepare only once one is already spent. Caps
+ // prepare residency at one frame of overshoot per budget (worst case
+ // `MAX_PARKED_BYTES` + `max_message_size`, 80 MiB per shard) instead
of
+ // at the budget, and is what makes an oversize frame parkable at all.
+ let namespace_budget_spent = parked_len > 0
+ && if replicated {
+ namespace_bytes >= MAX_PARKED_BYTES_PER_NAMESPACE
+ } else {
+ namespace_bytes.saturating_add(frame_cost) >
MAX_PARKED_BYTES_PER_NAMESPACE
+ };
+ let shard_budget_spent = if replicated {
+ parked_bytes >= MAX_PARKED_BYTES
+ } else {
+ parked_bytes.saturating_add(frame_cost) > MAX_PARKED_BYTES
+ };
+ if parked_len >= MAX_PARKED_PER_NAMESPACE || namespace_budget_spent ||
shard_budget_spent {
+ self.metrics.record_frame_drop(
+ crate::metrics::frame_drop_variant::PARTITION,
+ crate::metrics::frame_drop_reason::PARK_OVERFLOW,
);
- return ParkOutcome::Parked;
+ // Warn once per namespace on the transition into shedding, then
+ // `debug`. A full buffer is this branch's trigger, not a rate
limit:
+ // every later frame lands here too, and one formatted `warn`
apiece
+ // is enough for the non-blocking appender to shed unrelated lines.
+ // The counter carries the volume.
+ let first_shed = existing.is_none_or(|entry| {
Review Comment:
`is_none_or` returns true every time the namespace has no entry, and only
the shard-wide budget can shed with `existing == None` (the other two
conditions require a non-empty entry) - so on that path every frame warns and
`shed` is never recorded. that is exactly the load `MAX_PARKED_BYTES` was sized
for per its own doc, a burst against many un-materialised namespaces, so under
the intended scenario this warns per frame again: the log-loss the warn-once
change exists to prevent. the existing
`park_shard_wide_byte_budget_sheds_a_namespace_that_would_cross_it` test
already drives this path if you want to assert on it.
needs a shard-level gate for the entryless case - not
`entry().or_default()`, which would reintroduce the empty-entry-reads-as-parked
problem the comment above deliberately avoids.
--
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]
