feat(materialize): advance a backlogged window by a bounded prefix - #1649
Open
christophediprima wants to merge 2 commits into
Open
feat(materialize): advance a backlogged window by a bounded prefix#1649christophediprima wants to merge 2 commits into
christophediprima wants to merge 2 commits into
Conversation
The materialize watermark only advances when a whole pass succeeds. A pass that cannot finish its window writes no watermark, so the next pass re-reads a window one poll wider, and so on without bound. That is not merely slow, it is a one-way door. Once the window outgrows the source table's snapshot retention, the stored watermark can no longer be resolved, so every later poll falls back to a full table read — which is far more expensive and therefore even less likely to finish. The entry condition for the incremental path is exactly what is lost, so nothing recovers it. Measured on a 17-table deployment: one affected table became 13 of 17 in about four hours. 126 consecutive polls, every one "partial window, remainder deferred", zero completions, and every affected table reading its whole self on every poll. Resident memory rose 19.5 -> 24.7 GiB from the full reads alone. So bound the pass. `TableMetadata::window_end_capped` takes the OLDEST prefix of `(from, head]` — at most `FLUREE_MATERIALIZE_MAX_SNAPSHOTS_PER_PASS` snapshots, default 64 — and an unpinned materialize scan reads to there instead of to head. Three properties make this safe rather than merely smaller: - capping only ever moves `to` EARLIER, so no snapshot is skipped; the next poll resumes from the watermark this one wrote. - an initial full read (`from = None`) is never capped. A partial "full" read would be worse than an unbounded one. - any window error — expired ancestor, non-ancestor, rollback — falls through to head unchanged, leaving the existing full-read fallback to own that case. 64 is deliberately generous. A source committing 37-72 snapshots an hour stays under it at any sane poll interval, so a healthy job never reaches the cap; only a backlogged one does, and it then drains in bounded steps instead of never. `0` disables it and restores the previous behaviour exactly. The test asserts the DIRECTION explicitly (oldest prefix, not newest): taking the newest would skip everything between the watermark and the chosen end, which is silent data loss rather than a performance bug.
…backend The cap had three unit tests on `window_end_capped`, but the DECISION — head or prefix, and every way of declining — lived inline in `scan_for_materialize_stream` and had none. Testing it there needs a storage backend and an Iceberg fixture, which in this repo means `aws-testcontainers`; that is a high price for asserting a branch, and the usual result is that the branch stays unasserted. So move the decision into `TableMetadata::capped_scan_end`, where the metadata fixtures already exist. The scan path now calls it and uses the answer, which leaves the caller with no logic to get wrong and puts every decline path under test: - no snapshots at all, no panic - `max_snapshots == 0`, the disable switch - `from_id = None`, an initial full read - backlog already fits - the window cannot be walked (expired ancestor, non-ancestor, rollback) Two properties are asserted rather than assumed, because both are silent when wrong: the prefix is the OLDEST snapshots (taking the newest would skip everything between the watermark and the chosen end — data loss, not slowness), and the result never exceeds the head. Mutation-tested, each killed: taking the newest prefix, capping an initial full read, and dropping the fall-back-to-head on a window error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The failure this prevents
The materialize watermark only advances when a whole pass succeeds. A pass that cannot finish its window writes no watermark, so the next pass re-reads a window one poll wider, and so on without bound.
That is not merely slow — it is a one-way door:
Each turn makes the next more likely, and there is no exit: the entry condition for the incremental path — a resolvable
fromsnapshot — is exactly what gets lost.Measured on a 17-table deployment. One affected table became 13 of 17 in about four hours. 126 consecutive poll results, every one "partial window, remainder deferred", zero completions. All 13 tables showed an identical
from_snapshot_idacross nine consecutive polls: no watermark moved at all. Resident memory rose 19.5 → 24.7 GiB from the full-table reads alone.The tables were not individually unhealthy. They were dragged there by a shared property of the watermark, and once there they could not get back.
The change
TableMetadata::capped_scan_end(from_id, max_snapshots)returns where an unpinned incremental consumer should end this pass: the head, or an earlier snapshot when the backlog exceeds the cap. An unpinned materialize scan reads to there instead of to head.FLUREE_MATERIALIZE_MAX_SNAPSHOTS_PER_PASS, default 64;0disables it and restores the previous behaviour exactly.64 is deliberately generous. A source committing 37–72 snapshots an hour — ours does — stays under it at any sane poll interval, so a healthy job never reaches the cap and nothing about its behaviour changes. Only a backlogged job reaches it, and it then drains in bounded steps instead of never.
Why it is safe
Three properties, each asserted by a test rather than argued:
toearlier, never later, so no snapshot is skipped. The next pass resumes from the watermark this one wrote.from = None) is never capped. A partial "full" read is worse than an unbounded one — it would look complete while missing rows.The prefix is the oldest snapshots in the window, not the newest. That direction is the one thing here that is silent when wrong: taking the newest would skip everything between the watermark and the chosen end, which is data loss rather than a performance bug.
window_end_capped_takes_the_oldest_prefixexists to fail if anyone inverts it, and it does.On the second commit
The decision originally lived inline in
scan_for_materialize_stream. Testing it there needs a storage backend and an Iceberg fixture, which in this repo meansaws-testcontainers— a high price for asserting a branch, and the usual result is that the branch stays unasserted.The second commit moves it to
TableMetadata::capped_scan_end, beside the metadata fixtures, so the scan path calls it and has no logic of its own left to get wrong. Every decline path is then covered by an ordinary unit test: no snapshots, cap disabled, initial full read, backlog already fits, window unwalkable.Three mutations, each killed by exactly one test: taking the newest prefix instead of the oldest, capping an initial full read, and dropping the fall-back-to-head on a window error.
Scope, stated plainly
This prevents a table from entering that state. It does not rescue one already in it. A table whose watermark has already expired takes the full-read path, and this deliberately does not cap a full read — so it stays on that path until one read completes. The fix for the already-stuck case is sub-snapshot progress (a resumable file cursor), which is a larger change to the scan API and the persisted watermark, and belongs in its own PR.
We are happy to bring that too, and would rather agree the shape first — particularly whether a per-
(source, target, table)cursor is acceptable state growth, and whether it belongs in the state ledger or the target ledger. This PR stands on its own either way: it is the difference between a degradation that spreads and one that does not.