shuffle: discharge causal hints made unreachable by a byte gap - #3430
shuffle: discharge causal hints made unreachable by a byte gap#3430williamhbaker wants to merge 6 commits into
Conversation
| if offset == 0 { | ||
| self.pending_gap = true; | ||
| } |
There was a problem hiding this comment.
Is this an issue when the gap is a begin_mod_time cut rather than pruning? With a recent notBefore, the probe fast-forwards past every existing fragment, so offset == 0 sets pending_gap and sample_gap_floor then samples a clock of about now, putting the floor an hour ahead of the clocks the read is about to consume.
There was a problem hiding this comment.
More broadly, I don't think we want begin_mod_time cuts at all within this mechanism.
begin_mod_time is used to filter appends, but doesn't affect ACK propagation or causal hint resolution. It just suppresses the content of those transactions. I think we can totally ignore that begin_mod_time is a thing in this implementation, and focus just on offset jumps / gaps, and that it would still work because begin_mod_time sometimes causes such a gap (and sometimes doesn't, in which case no gap floor needs to be recorded).
There was a problem hiding this comment.
I had been trying (but not completely accomplishing, per Dave's comment) to avoid effectively turning off causal hints for the 1-hr of clock time following something like a collection truncation, where a begin_mod_time would regularly be applied. The additional fudge factor is needed to account for clocks not being guaranteed to be in ascending order in a journal, but that requires multiple producers and significantly different clocks amongst them.
Practically I think the extra buffer could be more like 10 minutes and be fine for actually doing what it needs to do, and then it would be less disruptive as well when applied to gaps caused by begin_mod_time. So I'll plan on doing that: Ignore begin_mod_time and tighten the extra padding from 1 hr to 10 minutes.
| /// below which its causal hints are discharged as unreachable | ||
| /// ([`frontier::Completed::is_gap_stale`]). | ||
| /// | ||
| /// Such a read may have found the journal's head pruned, which `notBefore` says |
There was a problem hiding this comment.
nit: perhaps do a de-duplication / don't repeat yourself pass over the comment prose?
There was a problem hiding this comment.
Done, made a few cleanups/improvements.
| if offset == 0 { | ||
| self.pending_gap = true; | ||
| } |
There was a problem hiding this comment.
More broadly, I don't think we want begin_mod_time cuts at all within this mechanism.
begin_mod_time is used to filter appends, but doesn't affect ACK propagation or causal hint resolution. It just suppresses the content of those transactions. I think we can totally ignore that begin_mod_time is a thing in this implementation, and focus just on offset jumps / gaps, and that it would still work because begin_mod_time sometimes causes such a gap (and sometimes doesn't, in which case no gap floor needs to be recorded).
| // An idempotent-recovery session must read through its hinted frontier | ||
| // exactly, so it does not observe gap floors. | ||
| if !recovery_session { | ||
| completed.advance_gap_floors(&resume_checkpoint.binding_gap_floors); |
There was a problem hiding this comment.
I'm not sure I'm following why this is required.
Also, what happens if idempotent replay content has been entirely pruned due to a gap?
There was a problem hiding this comment.
Agreed on this check not being required, and it would have prevented the ratchet from advancing when hints were discharged by a gap floor. I'll remove that.
Related to this and your question about idempotent replay content that has been entirely pruned...there's a similar check in on_progressed that I had over-applied here. I think that one is still necessary though, otherwise a recovery transaction could potentially discharge its own projected hints if the last_commits happen to fall near the gap.
This means that if idempotent replay content gets pruned between when the frontier is persisted and when it is replayed there is still a potential for a hint resolution stall. I've given that scenario quite a bit of thought but haven't come up with a solution I like very much.
A binding gap floor is a clock below which a binding's causal hints are unreachable. A read raises one when the broker resolves its start offset to a larger offset: a byte gap. `Frontier.binding_gap_floors` holds one floor per binding. A Progressed delta reports the floors its Slice raised. Checkpoint and Persist frontiers carry them onward. Go and Rust code is regenerated.
Add `binding_gap_floors` to `crate::Frontier`. It is keyed by binding index and handled like the backfill clocks: it reduces by per-binding maximum (`merge_binding_clocks`, renamed from `merge_backfill_clocks`), encodes and decodes with the proto field, rides the checkpoint peek, and is carried through `project_unresolved_hints`. Nothing raises or consumes a floor yet. Snapshots gain the empty field.
A read has a byte gap when the broker resolves its requested offset to a larger one. The read cannot receive the skipped bytes, and the response does not say why: retention removed them, `begin_mod_time` skipped them, or they were never written. Only a gap at read start raises a floor, when the start probe lands past the requested offset. `ReadState::sample_gap_floor` samples the first document the read then consumes, plus CAUSAL_HINT_GAP_MARGIN. Every ACK the gap removed precedes that document in offset order, so the sample bounds them all. The margin covers producer clock skew only, which is why it can be far smaller than PRODUCER_STALENESS_HORIZON. The cause of the gap does not matter, and `notBefore` is not consulted. `notBefore` suppresses document append but not commits, so hints resolve from any document the read receives. It bears on the floor only when its `begin_mod_time` fast-forward opens a gap. A gap mid-read raises nothing. `build_flush_frontier` drains each read's floor into the flush frontier, folding a binding's journals to their maximum. A floor raised at read start waits for the read's first document.
A causal hint names an ACK the session must read before its checkpoint resolves. If a byte gap put that ACK out of reach, no read can observe it. The session stalls until CAUSAL_HINT_RESOLUTION_TIMEOUT, restarts from the same checkpoint, and repeats. `Completed` gains the binding gap floor as a third authority. A hint below the floor is discharged as unreachable. Producer commits are never discharged: a byte gap says nothing about whether a producer committed, so a commit past its ceiling still freezes the ratchet. Floors ratchet from Progressed deltas ahead of promotion, because a byte gap is a fact about what a read can reach, not about a transaction's outcome. A delta carrying a floor re-prunes the pending checkpoint, promoting it if that clears its last hint. Every session seeds its floors from the resume checkpoint, so a durable floor accounts an incoming hint and the ratchet advances instead of freezing. But an idempotent-recovery session acts on no floor it raises while running. A floor cannot distinguish an ACK the gap removed from one just past the gap, and that session must replay its hinted frontier exactly. `prune_hints` returns a third count. The runtime-next tests are updated for the wider tuple.
Shard zero holds one `GF:{state_key}` row per binding. Every Frontier of
a Persist carries the same floors, so `encode_persist` writes each row
once, from whichever Frontier is present. The startup scan restores the
rows onto the committed Frontier (`restore_binding_clocks`, renamed from
`restore_backfill_clocks`). Neither `delete_*_frontier` clears them: a
floor is a retention fact, not transaction state.
The scan round-trip test now carries backfill clocks and a gap floor, so
the positional accumulator arguments of `RocksDB::scan` are checked
through a real scan.
Each leader actor keeps a session-cumulative map of gap floors, seeded from the recovered committed Frontier. It folds the floors of every Load frontier into the map, and stamps the map onto every Persist it issues. Unresolved peeks fold too, because a floor is a retention fact, not transaction state. Materialize's backfill markers still fold only from a resolved Load. The seed matters. A Persist overwrites each `GF:` row, and a Session's Load can report a floor below the durable one, so an unseeded session would lower the row on its first Persist. Materialize's hinted Persist precedes StartCommit, so no commit can carry read progress past a gap ahead of its floor. Derive's only Persist follows StartCommit, so a remote-authoritative derivation which crashes in between loses that transaction's floors.
3f98b52 to
8fa700a
Compare
|
Force-pushed changes based off review feedback, and to take care of a merge conflict. |
jgraettinger
left a comment
jgraettinger
left a comment
There was a problem hiding this comment.
contingent LGTM, though a concern below. I'd like to spend a bit more time thinking and talking about this, on the possibly-unfounded aspiration that there's a simpler approach
| ); | ||
|
|
||
| // A floor cannot tell an ACK the gap removed from one just past it, so | ||
| // the session which must replay its hinted frontier exactly acts on no |
There was a problem hiding this comment.
Trying to think through this case makes my head hurt 馃珷.
Per your comment before:
This means that if idempotent replay content gets pruned between when the frontier is persisted and when it is replayed there is still a potential for a hint resolution stall. I've given that scenario quite a bit of thought but haven't come up with a solution I like very much.
That makes sense, if the producer itself is gone and doesn't re-appear -- and we're in "backfill the binding" territory, so that's fine.
What happens if the hinted commit producer is still active, but the hinted commit has been pruned? In that case we may silently fold it's next ACK into the idempotent txn ... I think? As that's the first ACK that clears the hinted commit.
I'm not sure what, if anything, we do about this (nor am I confident I have the scenario right). I suppose the biggest actual concern is that it's a silent rather than noisy failure.
3 participants
Description:
A causal hint names an ACK that a session must read before its checkpoint resolves. A byte gap can remove that ACK: retention pruned it,
begin_mod_timeskipped it, or it was never written. No read can observe it. The session stalls untilCAUSAL_HINT_RESOLUTION_TIMEOUT, restarts from the same checkpoint, and repeats.This PR adds a binding gap floor: a clock below which a binding's causal hints are unreachable or irrelevant.
Frontier.binding_gap_floors, ratchet in the shuffle session, and persist asGF:{state_key}rows in shard zero. Each leader actor seeds its floors from the recovered committed Frontier.Of note are the cases intentionally NOT covered by this code:
notBeforeaddition racing an idempotent replay of a hinted checkpoint. Both of these could leave hints unreachable. The pruned fragment case has a rationale similar to the above point. ThenotBeforeaddition seems quite obscure and easy to remedy through user action.See individual commit messages for more detail.
Completed Q/A with a local stack: Drove the stack through various transitions and confirmed that the hint discharge and its floors are durable and operating as expected.
Closes #3414
Workflow steps:
(How does one use this feature, and how has it changed)
Documentation links affected:
(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)
Notes for reviewers:
(anything that might help someone review this PR)