Skip to content

feat(server): run the receiver data write path on the resolved Store (RFC 0019 slice 2c) - #298

Merged
jensholdgaard merged 2 commits into
mainfrom
rfc0019-green-receiver-store-migration
Jun 27, 2026
Merged

feat(server): run the receiver data write path on the resolved Store (RFC 0019 slice 2c)#298
jensholdgaard merged 2 commits into
mainfrom
rfc0019-green-receiver-store-migration

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented Jun 27, 2026

Copy link
Copy Markdown
Owner

What

Threads the resolved Store (local or S3) through the receiver's RFC 0014 data write path, so OTLP ingest lands Parquet on either backend (RFC 0019 slice 2c). This removes the receiver-on-s3 fail-fast that slice 2b (#297) added.

The pipeline was already Store-based — ParquetRecordSink::new(store, …) — but the receiver hardcoded local: ReceiverConfig took a bucket_root: PathBuf and serve() built Store::local from it. Now:

  • ReceiverConfig takes an opened Store instead of bucket_root.
  • serve() uses it directly — dropping its own create_dir_all + Store::local (the server already pre-creates the local root and opens the store in its startup preflight).
  • main() opens the store once (the existing preflight) and clones it into the receiver (Store is Arc-backed, cheap) while moving the original into Compactor::new — receiver, querier, and compactor all share the one store. The receiver-on-s3 fail-fast is removed.

Invariants (CLAUDE.md §3.4 / §3.6)

  • The WAL stays local. Only Parquet/audit/manifest objects go to the store; the WAL remains under wal.root and snapshots under wal.root/snapshots on local disk, regardless of backend. rfc0013_6_wal_stays_local (binary-spawn, local backend) still passes.
  • No on-disk format change; ingest semantics unchanged on the local backend (still routes through the same Store-backed sink).

Scope / follow-ups

  • Enables RFC0019.3 (S3 ingest→query e2e) — the localstack acceptance test lands in slice 3.
  • The compaction audit sink (ParquetAuditSink) is still local-only → on s3 the compactor uses a no-op sink (logged). That's slice 2d, the last code seam before the slice-3 e2e flips RFC 0019 green.

Tests

ReceiverConfig is binary-internal (the receiver module is private), so only main() constructs it — no test churn. All ourios-server tests pass (34/0; incl. the binary-spawning rfc0013_6_wal_stays_local). No test asserted the removed fail-fast.

Local gate

cargo fmt --all --check, cargo clippy -p ourios-server --all-targets --all-features -D warnings, RUSTDOCFLAGS=-D warnings cargo doc -p ourios-server, cargo test -p ourios-server — all green.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • The server now supports starting the receiver with either local storage or object storage.
    • Storage access is initialized before the service begins accepting traffic, so startup failures happen earlier and more safely.
  • Bug Fixes

    • Removed the previous startup restriction that blocked the receiver from running with non-local storage.
    • Improved startup validation so local path checks still happen immediately while remote storage is verified only when first used.

Correction (per review): main() shares the one preflight-opened Store between the receiver (cloned) and the compactor (moved); the querier opens its own handle from the same StoreConfig inside querier::serve. (Comment in main.rs corrected accordingly.)

…(slice 2c)

The receiver's RFC 0014 write path was pinned to a local Store: ReceiverConfig
took a bucket_root PathBuf and serve() built Store::local from it, so main()
fail-fast-rejected the receiver on s3. The pipeline (ParquetRecordSink) is
already Store-based, so this just threads the resolved Store through:
ReceiverConfig now takes an opened Store, serve() uses it directly (the server
pre-creates the local root + opens the store in its preflight), and main()
clones the one opened store into the receiver and the compactor. Ingest now
lands Parquet on local or S3. The WAL stays local regardless (§3.6).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

ReceiverConfig replaces bucket_root: PathBuf with store: Store. The serve function now uses config.store directly for sink construction instead of creating and opening a local store. In main.rs, receiver wiring passes the preflight-opened store handle for both local and S3 backends, removing the prior StoreConfig::Local-only match and the S3 early-failure path.

Receiver Store Unification

Layer / File(s) Summary
ReceiverConfig contract and sink wiring
crates/ourios-server/src/receiver.rs
ReceiverConfig.bucket_root is replaced with pub store: Store; serve() uses config.store directly when constructing ParquetRecordSink/SharedParquetSink, removing local store creation and the local-backend-only assumption.
main.rs preflight and receiver wiring
crates/ourios-server/src/main.rs
Comments document the preflight store open and backend-specific deferral; receiver role wiring now clones the opened store handle into ReceiverConfig for all backends, eliminating the StoreConfig::Local match and S3 error path.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • jensholdgaard/ourios#289: Introduced StoreConfig::open() and the preflight store-opening seam in main.rs that this PR's receiver wiring now uses.
  • jensholdgaard/ourios#245: Modified ReceiverConfig and main.rs receiver wiring along the same bucket_root/sink lifecycle path that this PR refactors.
  • jensholdgaard/ourios#294: Introduced store-backed Writer/Reader constructors enabling the compactor to operate over Store, consistent with propagating an open Store handle into sinks here.

Poem

🐇 Hop, hop, no more local-only gate,
The store is opened early — never late.
S3 or local, the handle's shared with care,
No bucket_root path left dangling in the air.
One cloned store, one sink, one tidy trail — 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: threading the resolved Store through the receiver write path.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description covers the summary, related RFC/issue context, tests, and checklist items, even though it doesn't use the exact template headings.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rfc0019-green-receiver-store-migration

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@crates/ourios-server/src/main.rs`:
- Around line 331-342: Receiver startup wiring is not covered: add a focused
test around the server’s receiver setup path that verifies `receiver::serve`
receives the correct `ReceiverConfig.store` for both `StoreConfig::Local` and
`StoreConfig::S3`. Place the test near `receiver.rs` or the server module and
exercise the code that builds `ReceiverConfig` in
`build_receiver_config`/`receiver::serve`, asserting the store is threaded
through unchanged for both backends.
🪄 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: c35b4bda-2abe-4664-8514-a55d27264cd5

📥 Commits

Reviewing files that changed from the base of the PR and between b1a22be and a3464c2.

📒 Files selected for processing (2)
  • crates/ourios-server/src/main.rs
  • crates/ourios-server/src/receiver.rs

Comment thread crates/ourios-server/src/main.rs

Copilot AI 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.

Pull request overview

This PR updates the ourios-server binary so the OTLP receiver’s RFC 0014 data write path uses the already-resolved ourios_parquet::Store (local or S3) instead of hardcoding a local filesystem root. This aligns the receiver with the existing Store-based parquet sink and removes the prior “receiver on S3” startup restriction.

Changes:

  • Replace ReceiverConfig.bucket_root: PathBuf with ReceiverConfig.store: Store and wire the receiver sink directly to that store.
  • Remove receiver-side local-root creation + Store::local(...) bootstrap, relying on server startup preflight to open the store.
  • In main, share the preflight-opened Store with the receiver via store.clone() and drop the receiver-on-S3 fail-fast.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
crates/ourios-server/src/receiver.rs Receiver config now carries an opened Store; receiver sink writes via the resolved backend (local/S3).
crates/ourios-server/src/main.rs Store is preflight-opened once and cloned into the receiver; receiver no longer rejects S3.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/ourios-server/src/receiver.rs Outdated
Comment thread crates/ourios-server/src/main.rs Outdated
… fixes

Review: add a focused receiver::serve test (local backend) covering the new
ReceiverConfig.store plumbing — binds :0, drives the store-backed sink, shuts
down cleanly (multi-thread runtime, since serve uses block_in_place); S3 is the
slice-3 localstack e2e. Also: the receiver writes data+manifest objects (not
the audit stream — that's the compactor's still-local sink); and the preflight
store is shared by receiver+compactor, while the querier opens its own handle.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@jensholdgaard
jensholdgaard merged commit 63cb4ad into main Jun 27, 2026
22 checks passed
@jensholdgaard
jensholdgaard deleted the rfc0019-green-receiver-store-migration branch June 27, 2026 23:36
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.

2 participants