Skip to content

persist: fetch source parts concurrently - #37315

Merged
antiguru merged 4 commits into
mainfrom
claude/pr-recreation-main-kwc6s0
Jul 6, 2026
Merged

antiguru merged 4 commits into
mainfrom
claude/pr-recreation-main-kwc6s0

Conversation

@antiguru

@antiguru antiguru commented Jun 26, 2026

Copy link
Copy Markdown
Member

Motivation

Hydrating a shard with many small parts is bound by the blob-store round-trip rather than bandwidth. This lets shard_source_fetch issue several part fetches at once so the round-trips overlap.

What this does

The fetch operator stays an async timely operator. Two changes make it safe to complete fetches out of order:

  • Time-keyed capability bookkeeping (not FIFO). For each time a part was minted at, the operator retains a data capability and a completed-fetches capability and counts the outstanding fetches at that time. When the count reaches zero both capabilities drop, advancing the data frontier and releasing that time's leases. Results can now complete in any order without advancing the frontier past unproduced data.
  • Concurrent fetching. Up to persist_source_fetch_concurrency (default 1, i.e. the previous serial behavior) fetches run at once via a FuturesUnordered, each on a cheap per-call BatchFetcher clone that shares the schema cache. In-flight bytes stay bounded by the existing fetch semaphore inside fetch_leased_part.

On a missing blob the operator reports the error and freezes (retains all capabilities, stops draining results) exactly as before, so the frontier never advances past a part we failed to emit.

New tests cover data fetching, mid-stream shutdown, the listing- and fetch-path freezes, and the concurrent fetch and concurrent-freeze paths.

Relationship to the original PR

Recreates antiguru/materialize#181 on top of the existing async operators, intentionally omitting that PR's deasync refactor of the shard_source operators. The deasync-only follow-ups are therefore also not needed and not included:

  • the txn-wal read-worker park bound (only required because deasync moved listen polling into a raw tokio task that no longer wakes the timely worker — the async listen here still activates normally), and
  • the introspection-naming change (shard_source_descs_returnshard_source_descs) — the separate lease-return operator is retained, so the dataflow graph is unchanged.

Generated by Claude Code

@antiguru
antiguru marked this pull request as ready for review June 26, 2026 11:28
@antiguru
antiguru requested review from a team as code owners June 26, 2026 11:28

@petrosagg petrosagg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, with one simplification in capability tracking

Comment thread src/persist-client/src/operators/shard_source.rs Outdated
@antiguru
antiguru force-pushed the claude/pr-recreation-main-kwc6s0 branch from 8fd7381 to 516e306 Compare July 6, 2026 08:26
@antiguru
antiguru requested a review from petrosagg July 6, 2026 08:50
claude added 4 commits July 6, 2026 09:55
Hydrating a shard with many small parts is bound by the blob-store
round-trip rather than bandwidth. Let `shard_source_fetch` issue several
part fetches at once so the round-trips overlap.

The fetch operator stays an async timely operator. Two changes make it
safe to complete fetches out of order:

* Key the operator's capability bookkeeping by the time a part was minted
  at rather than by arrival (FIFO) order. For each time we retain a data
  and a completed-fetches capability and count the outstanding fetches at
  that time; when the count reaches zero both capabilities drop, advancing
  the data frontier and releasing that time's leases. Results can now
  complete in any order without advancing the frontier past unproduced
  data.

* Run up to `persist_source_fetch_concurrency` (default 1, i.e. the
  previous serial behavior) fetches at once via a `FuturesUnordered`, each
  on a cheap per-call `BatchFetcher` clone that shares the schema cache.
  In-flight bytes stay bounded by the existing fetch semaphore inside
  `fetch_leased_part`.

On a missing blob the operator reports the error and freezes (retains all
capabilities, stops draining results) exactly as before, so the frontier
never advances past a part we failed to emit. New tests cover data
fetching, mid-stream shutdown, the listing- and fetch-path freezes, and
the concurrent fetch and concurrent-freeze paths.

Recreates antiguru#181 on top of the async operators,
omitting that PR's deasync refactor of the shard_source operators (and
its dependent txn-wal and introspection-naming follow-ups).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MR6SwYEgWVmMJUGn4ZmLMX
`bin/lint-test-flags` requires new dyncfgs to be known to the test
harnesses. Add `persist_source_fetch_concurrency` to parallel-workload's
`FlipFlagsAction` and to `get_variable_system_parameters` so randomized
CI exercises the concurrent fetch path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MR6SwYEgWVmMJUGn4ZmLMX
Address review feedback: rather than tracking outstanding fetches in a
time-keyed `BTreeMap` with manual counting and `expect`s, carry each
part's input capabilities through the pending queue and the in-flight
future. Timely then tracks the frontier for us: it advances past a time
only once every fetch minted at that time has completed and dropped its
capability clones. On a missing blob we still freeze by never returning
from `report_and_stop`, which retains every in-flight and pending
capability so the frontier cannot advance past the unemitted part.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MR6SwYEgWVmMJUGn4ZmLMX
Drop the completed-fetches capability directly in the match pattern
(`[cap, _]`) and let the retained data capability fall out of scope,
removing the explicit `drop` and the redundant comment. The error path
still holds the data capability across the never-returning
`report_and_stop`, so the freeze is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MR6SwYEgWVmMJUGn4ZmLMX
@antiguru
antiguru force-pushed the claude/pr-recreation-main-kwc6s0 branch from c256406 to 322d82a Compare July 6, 2026 09:57
@antiguru
antiguru enabled auto-merge (squash) July 6, 2026 10:00
@antiguru
antiguru merged commit 70f75e4 into main Jul 6, 2026
120 of 121 checks passed
@antiguru
antiguru deleted the claude/pr-recreation-main-kwc6s0 branch July 6, 2026 10:35
@antiguru

antiguru commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

Thanks for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants