fix(index): fine-tune fjall default for expected write throughput#896
fix(index): fine-tune fjall default for expected write throughput#896
Conversation
📝 WalkthroughWalkthroughThis PR replaces the derived Default implementation for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/config.rs (1)
354-392:⚠️ Potential issue | 🟠 MajorPer-field
#[serde(default)]bypasses the new defaults during partial deserialization.Each field carries
#[serde(default)], which means serde usesOption::default()(i.e.,None) for every omitted field — not the values from your manualDefaultimpl. The custom defaults only take effect when the entireFjallIndexConfigis absent andIndexStoreConfig::default()is called. If a user supplies a partial config (e.g., onlypath), the tuned values formax_journal_size,worker_threads, etc. will silently beNone.Replace the per-field
#[serde(default)]annotations with a single struct-level#[serde(default)]so that missing fields fall back to theDefaultimpl:Proposed fix
-#[derive(Serialize, Deserialize, Clone, Debug)] +#[derive(Serialize, Deserialize, Clone, Debug)] +#[serde(default)] pub struct FjallIndexConfig { /// Optional path override. If relative, resolved from storage root. /// If not specified, defaults to `<storage.path>/index`. - #[serde(default)] pub path: Option<PathBuf>, /// Size (in MB) of memory allocated for caching. - #[serde(default)] pub cache: Option<usize>, /// Maximum journal size in MB (default: 1024). - #[serde(default)] pub max_journal_size: Option<usize>, /// Flush journal after each commit (default: false). - #[serde(default)] pub flush_on_commit: Option<bool>, /// L0 compaction threshold (default: 8, lower = more aggressive). - #[serde(default)] pub l0_threshold: Option<u8>, /// Number of background compaction worker threads (default: 8). - #[serde(default)] pub worker_threads: Option<usize>, /// Memtable size in MB before flush (default: 128). - #[serde(default)] pub memtable_size_mb: Option<usize>, }
🧹 Nitpick comments (1)
crates/core/src/config.rs (1)
209-236: Consider applying similar tuned defaults toFjallStateConfig.
FjallStateConfighas the same set of tunable fields (max_journal_size,l0_threshold,worker_threads,memtable_size_mb, etc.) but still derivesDefault, so all fields default toNone. The doc comments even reference non-None defaults (e.g., "default: 4" forl0_threshold, "default: 64" formemtable_size_mb) that don't match the derived behavior. If the index store benefits from explicit defaults for write throughput, the state store likely does too.
Summary by CodeRabbit