docs(rfc): rfc 0035 — ingest concurrency (relax global miner serialization) - #572
Merged
Conversation
📝 WalkthroughWalkthroughRFC 0035 documents a two-phase ingest design that preserves WAL and template-id ordering while moving Parquet encoding outside the global critical section. It defines an encode-drain barrier, alternatives, acceptance scenarios, testing guidance, open questions, references, and adds the RFC to the documentation index. ChangesIngest concurrency RFC
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ation) Drafts the design to remove the global commit-gate + miner-lock serialization found in issue #571 (profile: ~86k lines/s at 1.2/8 cores, 85% idle). Recommends Design A — keep template-id assignment globally ordered under the gate, move the order-insensitive Parquet encoding onto a concurrent pool — capturing the idle headroom with no on-disk format change. Documents and defers Design B (per-tenant id-space, §3.5 migration). Sequences RFC 0034's D1 recalibration after this RFC's measured number. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
jensholdgaard
force-pushed
the
rfc-0035-ingest-concurrency
branch
from
July 20, 2026 13:45
9557a0b to
45d7ad2
Compare
jensholdgaard
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Adds RFC 0035 to the documentation set, describing a design to remove ingest hot-path serialization by keeping WAL durability ordering intact while relaxing the expensive post-mining work to improve multi-tenant throughput.
Changes:
- Add a new draft RFC: RFC 0035 — Ingest concurrency (design-focused, no code changes).
- Link RFC 0035 into the mdBook navigation (
docs/SUMMARY.md).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| docs/SUMMARY.md | Adds RFC 0035 to the RFC index in the mdBook summary. |
| docs/rfcs/0035-ingest-concurrency.md | Introduces RFC 0035 describing Design A (ordered id assignment + concurrent Parquet encode) and defers Design B (per-tenant id-space + migration). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rfcs/0035-ingest-concurrency.md`:
- Around line 224-251: The RFC is missing the required Testing strategy section
between Measurements and Open questions. Add a section describing deterministic
replay, concurrent multi-tenant, rotation/restore, durability, sink-concurrency,
and benchmark tests, then renumber all subsequent section headings and
references consistently.
- Around line 96-121: Define an explicit WAL high-water or epoch barrier between
the ordered phase and concurrent encoders, and make rotation wait until all
encodes for the covered range finish before snapshotting trees or closing the
sink. Ensure records cannot write to a closed or newer-generation sink, and
propagate worker encoding errors through ingest and rotation so coherence is not
reported unless the drain succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 36b3f1e4-13d1-4e52-b9c5-315c39d2ae06
📒 Files selected for processing (2)
docs/SUMMARY.mddocs/rfcs/0035-ingest-concurrency.md
…arrier, status specified Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
docs/rfcs/0035-ingest-concurrency.md:30
- The lifecycle description here implies §6 (Testing strategy) is part of what "places" the RFC at
specified, butdocs/rfcs/README.mddefinesspecifiedbased on §5 acceptance criteria being written and review confirming testability in principle. Rewording this sentence to match the documented lifecycle avoids teaching an incorrect gate.
> `red`→`green`. Per `docs/rfcs/README.md`, §§1–4 are the design
> contract, §5 the acceptance criteria and §6 the testing strategy — all
> written, which places this RFC at **`specified`**; this PR's review
> confirms the criteria are testable, after which `red` (failing stubs)
> begins. §7 lists what a prototype must still resolve before
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Snapshot-restore determinism is RFC 0001 §3.5.3, a scenario id there; CLAUDE.md has no §3.5.3. Fixes the how-to-read invariant list, the §2.3 divergence reference, the §7 meta-RFC checklist item, and the §8 CLAUDE.md reference line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rfcs/0035-ingest-concurrency.md`:
- Around line 18-21: Narrow the compatibility claim in
docs/rfcs/0035-ingest-concurrency.md lines 18-21 by replacing “changes no
on-disk byte” with a guarantee of schema, representation, template-ID, and
query-result compatibility. In docs/rfcs/0035-ingest-concurrency.md lines
155-160, explicitly state that concurrent encoding may change row order and
Parquet bytes while preserving those semantics and schema guarantees.
- Around line 139-145: Update RFC0035.2’s rotation and shutdown ordering so the
encode pool first drains through the rotation offset, then RecordSink flush_all
is invoked and durable store writes are awaited successfully, and only afterward
wal_high_water is advanced. Add or update the test to assert this ordering and
ensure the high-water mark is not stamped when flushing or durable writes fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7db5cc0a-7873-4f94-b584-8469bb385e1c
📒 Files selected for processing (2)
docs/SUMMARY.mddocs/rfcs/0035-ingest-concurrency.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/SUMMARY.md
…ope byte-identity out Two review fixes: (1) the encode-drain barrier is strengthened to an encode-drain-AND-flush barrier — emit leaves records in an in-memory partition buffer, and recovery replays only above the high-water, so the barrier must durably flush partitions holding a record <= mark before stamping wal_high_water. (2) RFC0035.5 explicitly disclaims byte-for-byte Parquet identity (concurrent encode may reorder rows); the guarantee is schema + semantic + query-result stability, asserted as multiset row equality, never a file hash. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
…orthcoming Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
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.
What
specifiedRFC 0035 — the design to remove the ingest hot-path serialization found in #571 (profile: ~86k lines/s while using 1.2 of 8 cores, ~85% idle; root causepipeline.rs:314-354— a global commit-gate + global miner lock, with the expensive Parquet encode inside the single-file section).This is the "pursue the 6×" workstream. Per CLAUDE.md §5.1 it starts with an RFC because it touches the highest-risk invariants (§3.4 WAL durability, RFC 0001 §3.5.3 miner determinism, §3.7 tenancy).
Two orderings, only one relaxed: WAL append + fsync stay globally ordered (durability, untouched); the miner hand-off relaxes. It recommends:
template_idrepresentation, the Parquet schema, or the query surface.template_idmigration (§3.5) rippling into the DSL and snapshot format — a large separate commitment, revisited only if A's measured ceiling leaves D1 unmet.The map corrected my first instinct —
next_template_idis a single cluster-wide counter, so per-tenant mining would change assigned id values vs. WAL-order replay. Design A sidesteps that by keeping id assignment ordered; only the encode moves. Review then surfaced a real correctness addition: an encode-drain-and-flush barrier — becauseemitonly buffers records in memory and recovery replays only above the high-water, the barrier must durably flush partitions holding a record≤ markbefore advancingwal_high_water.Status / structure
status: specified— §§1–4 design contract, §5 acceptance criteria (5 scenarios), and §6 Testing strategy are all written (per docs/rfcs/README.md, that is what places it atspecified). This PR's review confirms the criteria are testable;red(failing stubs) follows.§6's target multiple is deliberately unfilled — it comes from a prototype + baseline measurement at
red(§7), which also sets RFC 0034's recalibrated D1 bar. No code changes here; this is the design contract for review.Sequencing
RFC 0034 (D1 re-scope) is a held local draft, sequenced after this RFC — recalibrate D1 against Design A's measured number. Not yet in-tree.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y