Skip to content

Update litestream configuration#729

Merged
epompeii merged 2 commits intodevelfrom
u/ep/litestream-config
Mar 24, 2026
Merged

Update litestream configuration#729
epompeii merged 2 commits intodevelfrom
u/ep/litestream-config

Conversation

@epompeii
Copy link
Copy Markdown
Member

This changeset continues the litestream configuration updates following:

  1. Upgrade to Litestream 5.x #719
  2. Add Litestream checkpoint configuration settings #727

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 23, 2026

🤖 Claude Code Review

PR: #729
Base: devel
Head: u/ep/litestream-config
Commit: be52826c0bee60ba085272ffd86d9223cd1b7e91


Good — no remaining references to the old field names. Here's the review:


PR Review: Restructure Litestream snapshot/validation config into nested objects

Summary

This PR refactors the JsonLitestream config from flat fields (snapshot_interval, retention, validation_interval) into nested structs (JsonSnapshot, JsonValidation), mirroring the existing pattern established by JsonCheckpoint. Two commits: json_snapshot and from.

Positives

  • Good structural alignment: Grouping interval and retention under JsonSnapshot (and interval under JsonValidation) mirrors how JsonCheckpoint is already structured. This is more intuitive.
  • Proper destructuring: From impls and into_yaml use let Self { .. } = self; pattern, per project conventions.
  • Thorough test updates: All 5 YAML generation tests are updated. The snapshot+validation test verifies the correct YAML output.
  • Docs and OpenAPI spec updated: All 9 locale docs and openapi.json are consistent with the Rust changes.
  • No stale references: No remaining uses of the old flat field names anywhere in the codebase.

Issues

1. Breaking API change — no migration path (Medium)

This is a breaking change to the server config schema. Existing bencher.json configs using the old flat fields (snapshot_interval, retention, validation_interval) will silently ignore those fields after upgrading — serde will skip unknown fields by default. Users would lose their snapshot/validation settings without any error or warning.

Suggestion: Consider adding #[serde(alias = "...")] or a custom deserializer that maps old field names, or at minimum document the breaking change in release notes.

2. Docs removed retention_check_interval (Low, pre-existing)

The old docs listed retention_check_interval as a configurable option, but it was never in the Rust struct. The new docs correctly remove it. This is fine — just noting it was a pre-existing doc/code mismatch that's now cleaned up.

3. JsonSnapshot with all-None fields serializes as empty object (Nit)

A JsonSnapshot { interval: None, retention: None } will serialize to snapshot: {} in YAML. The old code would produce None (no snapshot section) in this case. This is a minor behavioral difference that's unlikely to matter in practice — Litestream should handle an empty snapshot section gracefully — but worth noting.

4. Commit messages are terse

The commit messages from and json_snapshot don't describe what changed or why. Consider more descriptive messages for reviewability (e.g., "Restructure Litestream snapshot/validation config into nested objects").

Code Quality

  • Follows project conventions (#[cfg_attr(feature = "schema", derive(JsonSchema))], skip_serializing_if, destructuring)
  • No lint suppressions added
  • No security concerns — this is config struct reshaping only
  • No performance implications

Verdict

Clean refactoring that improves config ergonomics. The main concern is silent breakage for existing users (issue #1). If this is a Plus-only server config that's not widely deployed, the risk is lower, but a serde compatibility shim or migration note would be prudent.


Model: claude-opus-4-6

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 23, 2026

🐰 Bencher Report

Branchu/ep/litestream-config
Testbedubuntu-22.04
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
microseconds (µs)
(Result Δ%)
Upper Boundary
microseconds (µs)
(Limit %)
Adapter::Json📈 view plot
🚷 view threshold
3.78 µs
(+5.79%)Baseline: 3.57 µs
4.69 µs
(80.62%)
Adapter::Magic (JSON)📈 view plot
🚷 view threshold
3.86 µs
(+9.16%)Baseline: 3.54 µs
4.62 µs
(83.56%)
Adapter::Magic (Rust)📈 view plot
🚷 view threshold
25.03 µs
(-1.58%)Baseline: 25.43 µs
31.67 µs
(79.04%)
Adapter::Rust📈 view plot
🚷 view threshold
2.83 µs
(+1.30%)Baseline: 2.80 µs
3.22 µs
(87.99%)
Adapter::RustBench📈 view plot
🚷 view threshold
2.84 µs
(+1.74%)Baseline: 2.80 µs
3.22 µs
(88.29%)
head_version_insert/batch/10📈 view plot
🚷 view threshold
107.56 µs
(+6.96%)Baseline: 100.56 µs
118.11 µs
(91.07%)
head_version_insert/batch/100📈 view plot
🚷 view threshold
242.27 µs
(+1.67%)Baseline: 238.28 µs
264.46 µs
(91.61%)
head_version_insert/batch/255📈 view plot
🚷 view threshold
471.03 µs
(+1.68%)Baseline: 463.23 µs
498.01 µs
(94.58%)
head_version_insert/batch/50📈 view plot
🚷 view threshold
170.42 µs
(+5.57%)Baseline: 161.43 µs
182.15 µs
(93.56%)
threshold_query/join/10📈 view plot
🚷 view threshold
150.03 µs
(+3.14%)Baseline: 145.46 µs
168.90 µs
(88.83%)
threshold_query/join/20📈 view plot
🚷 view threshold
166.03 µs
(+3.78%)Baseline: 159.98 µs
185.68 µs
(89.42%)
threshold_query/join/5📈 view plot
🚷 view threshold
145.84 µs
(+6.01%)Baseline: 137.57 µs
159.68 µs
(91.33%)
threshold_query/join/50📈 view plot
🚷 view threshold
206.11 µs
(+2.36%)Baseline: 201.35 µs
230.54 µs
(89.40%)
🐰 View full continuous benchmarking report in Bencher

@epompeii epompeii force-pushed the u/ep/litestream-config branch from 7252422 to be52826 Compare March 24, 2026 02:06
@epompeii epompeii merged commit 2ab39d5 into devel Mar 24, 2026
61 of 62 checks passed
@epompeii epompeii deleted the u/ep/litestream-config branch March 24, 2026 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant