Gate lz4-sys behind an optional lz4-compression feature#397
Open
humancto wants to merge 3 commits intotikv:masterfrom
Open
Gate lz4-sys behind an optional lz4-compression feature#397humancto wants to merge 3 commits intotikv:masterfrom
humancto wants to merge 3 commits intotikv:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @humancto. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
needs-ok-to-test
|
Welcome @humancto! |
📝 WalkthroughWalkthroughAdds an optional, default-on Cargo feature ChangesLZ4 Feature and Runtime API
CI: No-default-features Build/Test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
ti-chi-bot
Bot
added
size/L
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/rust.yml (1)
41-42: No-default-features step doesn't exercise the new gated test.This step only runs
cargo buildon the default target, so the#[cfg(not(feature = "lz4-compression"))]test added insrc/config.rs(test_sanitize_rejects_nonzero_threshold_without_lz4_feature) is never compiled or executed anywhere in CI — it's excluded from the default-featuremake testrun and skipped here. Also,cargo buildwon't compile test/bench/example targets, so regressions in stub-related code paths under feature-off can slip through.Consider upgrading the step to
cargo check --no-default-features --all-targetsand/orcargo test --no-default-featuresso the sanitize-rejection test and any feature-off test code actually run.🔧 Suggested change
- - name: Build (no default features) - run: cargo build --no-default-features --verbose + - name: Build and test (no default features) + run: | + cargo check --no-default-features --all-targets --verbose + cargo test --no-default-features --verbose🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rust.yml around lines 41 - 42, The "Build (no default features)" CI step currently runs only `cargo build --no-default-features --verbose` so tests gated by the lz4 feature (e.g., the #[cfg(not(feature = "lz4-compression"))] test test_sanitize_rejects_nonzero_threshold_without_lz4_feature in src/config.rs) are never compiled or run; update that step to run a command that compiles and/or runs tests for all targets without default features such as `cargo check --no-default-features --all-targets` and/or `cargo test --no-default-features` so the gated test and any non-default-feature test code are exercised in CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/rust.yml:
- Around line 41-42: The "Build (no default features)" CI step currently runs
only `cargo build --no-default-features --verbose` so tests gated by the lz4
feature (e.g., the #[cfg(not(feature = "lz4-compression"))] test
test_sanitize_rejects_nonzero_threshold_without_lz4_feature in src/config.rs)
are never compiled or run; update that step to run a command that compiles
and/or runs tests for all targets without default features such as `cargo check
--no-default-features --all-targets` and/or `cargo test --no-default-features`
so the gated test and any non-default-feature test code are exercised in CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bbd4ff3-2505-4eec-a480-3a1e0e448cb8
📒 Files selected for processing (6)
.github/workflows/rust.ymlCHANGELOG.mdCargo.tomlREADME.mdsrc/config.rssrc/util.rs
humancto
added a commit
to humancto/mango
that referenced
this pull request
Apr 24, 2026
* chore(adr): record raft-engine fork for lz4-sys feature-gate (W5) ADR 0002 §W5 previously assumed upstream raft-engine's lz4-sys C FFI dependency could be mitigated purely at the data path - by disabling raft-engine's own compression and doing it above the engine in mango-raft using lz4_flex. That remains true for the data path, but the build-graph concern - upstream's unconditional lz4-sys dependency pulling the C toolchain into mango's supply chain - was unsolved by the data-path mitigation alone. This commit records the build-time fix: 1. ADR 0002 §W5 expanded to document the active fork (humancto/raft-engine at SHA e1d738d), the upstream PR that tracks its retirement (tikv/raft-engine#397), and the explicit consumer contract (default-features = false, feature list, and the Config::sanitize reject enforcing batch-compression-threshold = 0). 2. New file .planning/fork-raft-engine-lz4-verification.md records the full fork state: SHA pins, PR URL, commit footprint, how mango consumes the fork, rebase policy while the fork lives, retirement plan when #397 merges, and supply-chain audit posture. The fork itself was adversarially reviewed by rust-expert across two rounds (REVISE -> APPROVE_WITH_NITS). Upstream PR #397 is open. No mango Cargo.toml changes here - the workspace dep entry lands with PR-1 (mango-storage skeleton) which pins the fork SHA. * docs(fork-tracking): address rust-expert APPROVE_WITH_NITS must-fix items rust-expert's review of PR #48 returned APPROVE_WITH_NITS with two must-fix items on .planning/fork-raft-engine-lz4-verification.md. Both are text-only and localized; applying them here to close out the review without another round. 1. Retirement branches for non-merge endings. Original text assumed upstream PR #397 merges. Added a new "What if upstream rejects the approach or stalls" section with two branches: (a) #397 closed without merge, upstream proposes a different shape - swap the tracked PR, follow the same 6-step retirement once the replacement merges. (b) #397 stalls for 12+ months - escalate to ADR 0002 §W4 escape hatch (b) and vendor the pinned fork SHA under crates/vendored-raft-engine/. 2. Supply-chain audit posture corrected. Original paragraph conflated cargo-vet's exemption-identity behavior with SHA pinning and review-by annotations. Rewritten to spell out the actual semantics: - Exemptions key on (crate_name, crate_version); the fork and upstream both ship package.version = "0.4.2", so one exemption line covers both while the fork is active and after retirement. - Patch-version bumps (raft-engine historically bumps on master without publishing) break matching and need a new version line. - SHA swaps are silent to vet - no re-exemption needed on rebase. - review-by is a mango convention, not a vet behavior; dates need manual refresh on rebase and on retirement. Per rust-expert's explicit guidance, no further review round is needed for these text-only edits. --------- Co-authored-by: Archith <archith.rapaka@atomtickets.com>
humancto
added a commit
to humancto/mango
that referenced
this pull request
Apr 24, 2026
…k (ROADMAP:815) (#49) * feat(storage): add mango-storage skeleton with redb + raft-engine fork deps Adds the Phase 1 storage crate skeleton per ROADMAP:815. Scaffolding only — no trait definitions yet (those land in ROADMAP:816's own PR), no implementations. The goal of this PR is to prove the two load-bearing storage dependencies integrate cleanly end to end (resolver, cargo-deny, cargo-vet, cargo-geiger) before the trait and impl PRs land on top. What ships: - crates/mango-storage skeleton: publish = false, deny(missing_docs), matches the shape of the existing mango crate. Single VERSION-matches-manifest smoke test. - Cargo.toml workspace deps: redb = "4.1.0" raft-engine = humancto/raft-engine at e1d738d, default-features = false, features = ["internals"] Consumers inherit the feature set verbatim via `.workspace = true`. Dropped the `scripting` feature from the original ADR 0002 §W5 plan: scripting pulls rhai → smartstring (MPL-2.0), which would require a license-allowlist widening for a raft-engine surface mango does not use (the TiKV-admin CLI path). `internals` is the only feature mango- raft actually needs. - deny.toml: - allow-git allowlist entry for humancto/raft-engine with a retirement trigger (tikv/raft-engine#397 merging → drop the entry). - skip entries for four raft-engine-transitive major-version gaps (bitflags 1.x, getrandom 0.2, hashbrown 0.14, syn 1.x). Each names the retirement trigger (raft-engine upstream refresh). - ignore entry for RUSTSEC-2024-0437 (protobuf 2.28 uncontrolled recursion via raft-engine → prometheus). Skeleton exercises zero protobuf parsing; mango-raft impl PRs will carry fuzz corpora for the crafted-input class per ROADMAP's Reviewer's contract. - supply-chain/config.toml: - [policy.mango-storage] first-party stanza. - [policy.raft-engine] audit-as-crates-io = true so one [[exemptions.raft-engine]] version = "0.4.2" entry covers both the active fork and post-retirement upstream, matching the posture recorded in .planning/fork-raft-engine-lz4-verification.md. - ~90 new [[exemptions.*]] entries for the transitive graph of both deps, each with review-by: 2026-10-23 in notes per house convention. - unsafe-baseline.json: add mango-storage (all zeros — skeleton has no unsafe, no #![allow(unsafe_code)], no miri enrollment). - supply-chain/imports.lock: auto-bumped by `cargo vet regenerate exemptions`. - .planning/mango-storage-skeleton.plan.md: the revised plan after rust-expert's REVISE review (addressed two showstoppers, three bugs, and eight missing items from the first-round findings). Verification (local): cargo check --workspace OK cargo clippy --workspace --all-targets -- -D warnings OK cargo fmt --check OK cargo nextest run -p mango-storage 1 passed cargo deny check (advisories/bans/licenses/sources) all OK cargo vet check Succeeded cargo run -q -p xtask-vet-ttl PASS cargo tree -p mango-storage -i lz4-sys lz4-sys absent Closes ROADMAP:815. Next item: ROADMAP:816 (Backend + RaftLogStore trait definition). * fix(storage): address rust-expert REVISE must-fix items on PR #49 Addresses the five must-fix items from rust-expert's final diff review before the mango-storage skeleton can merge: 1. audit.yml: add `ignore: RUSTSEC-2024-0437` — rustsec/audit-check does not read deny.toml, so the protobuf recursion advisory must be suppressed separately here. Cross-reference comment points at deny.toml as the single source of truth for the justification. 2. fork-raft-engine-lz4-verification.md: rewrite the manifest snippet and retirement snippet to show `features = ["internals"]`, add a "scripting dropped" rationale section (MPL-2.0 via rhai / smartstring, avoided by not taking a feature mango does not use), and bump "Last updated" to reflect that PR #49 wires the dep. 3. supply-chain/config.toml raft-engine exemption: keep the SHA-qualified `version = "0.4.2@git:<SHA>"` form — verified by running `cargo vet check` that plain `version = "0.4.2"` does NOT match a git source, even with `audit-as-crates-io = true`. Rewrite the exemption notes AND the `[policy.raft-engine]` notes to honestly describe the actual cargo-vet behavior (exemptions key on fully-resolved source, not `package.version` alone) and the rebase/retirement implications. Fork-tracking doc §"Supply-chain audit posture" rewritten to match. 4. supply-chain/config.toml crc32fast exemption: add the missing `notes = "review-by: 2026-10-23 — raft-engine 0.4.2 transitive, audit pending"`. xtask-vet-ttl would have caught this on any next run. 5. mango-storage-skeleton.plan.md: update the Goal section and the raft-engine pin snippet to show `features = ["internals"]`, add a "Deviations from plan" section at the bottom enumerating the four real implementation departures from the revised plan (scripting drop, explicit version on the git dep, four dedup-skip entries, RUSTSEC-2024-0437 ignore). Verified locally: - cargo vet check — PASS - cargo deny check — advisories/bans/licenses/sources all ok - cargo run -q -p xtask-vet-ttl — PASS (149/149 exemptions have review-by) - cargo clippy --workspace --all-targets -- -D warnings — clean - cargo fmt --check — clean imports.lock drift is cargo-vet's auto-regeneration; included in the commit to keep the working tree clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(storage): cargo-vet fmt — triple-quote the raft-engine policy notes CI surfaced: cargo-vet 0.10.2 store-consistency check rejects a single-quoted multi-line string that contains an inner escape-quote. cargo vet fmt (which CI runs with --locked --frozen, and which rejects any reformatting delta) wanted the notes rewritten as a TOML triple-quoted literal. This is a formatting-only change: the text content is unchanged. Local cargo vet check PASSES. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Archith <archith.rapaka@atomtickets.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
humancto
pushed a commit
to humancto/raft-engine
that referenced
this pull request
May 3, 2026
CodeRabbit review on PR tikv#397 flagged that the new 'Build (no default features)' CI step ran only 'cargo build', which neither compiles nor runs the test_sanitize_rejects_nonzero_threshold_without_lz4_feature test in src/config.rs (gated #[cfg(not(feature = "lz4-compression"))]). The test silently never ran in any CI job, defeating the point of adding it. Replaces the step with: cargo check --no-default-features --all-targets --verbose cargo test --no-default-features --verbose --all-targets covers tests/benches/examples in the no-default-features graph; the test invocation actually exercises the gated reject + accept paths in Config::sanitize. Signed-off-by: Archith <archith.rapaka@gmail.com>
Author
|
Addressed the CodeRabbit nitpick on the no-default-features CI step in ea4e85c. The previous - name: Build and test (no default features)
run: |
cargo check --no-default-features --all-targets --verbose
cargo test --no-default-features --verbose
|
Gate the `lz4-sys` C FFI dependency behind a new `lz4-compression` Cargo feature, default-on so existing downstream behaviour is preserved bit-for-bit. Motivation ---------- Consumers who wish to build raft-engine as part of a pure-Rust tree currently cannot: `lz4-sys = "=1.9.5"` is a hard, unconditional dependency that pulls in a C compiler step at build time. Making the C dependency feature-gated lets such consumers opt out via `default-features = false` (plus any other features they need), without affecting anyone who stays on defaults. Mechanism --------- - `Cargo.toml`: mark `lz4-sys` as `optional = true` and expose a new `lz4-compression` feature that enables it via the namespaced `dep:lz4-sys` syntax. `default = [..., "lz4-compression"]` keeps existing users on the same code path with no action required. - `src/util.rs`: split `pub mod lz4` into two cfg-gated variants. The real implementation stays behind `#[cfg(feature = "lz4-compression")]`; a stub module with an identical public surface (`DEFAULT_LZ4_COMPRESSION_LEVEL`, `append_compress_block`, `decompress_block`) is provided under `#[cfg(not(feature = "lz4-compression"))]`. The stub returns `Error::Other` on attempted compression (with guidance to rebuild or set `compression_threshold = 0`) and `Error::Corruption` on attempted decompression (reader cannot interpret LZ4 payload). Call sites in `log_batch.rs` compile unchanged under both arms. Operator contract ----------------- Callers who disable this feature: - MUST set `compression_threshold = 0` in their `Config` to avoid the runtime `append_compress_block` error path. - MUST NOT open data files written by a build that had LZ4 compression active; those entries will surface as `Corruption` on read. Verified -------- - `cargo build --no-default-features`: compiles, `cargo tree --no-default-features --edges no-dev | grep lz4` is empty (lz4-sys fully absent from the build graph). - `cargo build` (defaults): compiles, `cargo tree --edges no-dev | grep lz4` still shows `lz4-sys v1.9.5` — unchanged behaviour for existing consumers. - Full `cargo test` run blocked locally by a pre-existing grpcio-sys cmake toolchain issue on this workstation unrelated to this patch; relying on upstream CI to exercise the lz4 test module under the default feature set. Signed-off-by: Archith <archith.rapaka@gmail.com>
rust-expert's adversarial review of 4e8a088 flagged six changes needed before the patch is upstream-acceptable. This commit applies all six. 1. Fail-fast on misconfigured threshold (Showstopper) Config::sanitize now returns Err when batch_compression_threshold > 0 while the lz4-compression feature is disabled, instead of letting every >= 8KB log batch silently fail at write time. Matches the existing error patterns for enable_log_recycle / prefill_for_recycle. New unit test test_sanitize_rejects_nonzero_threshold_without_lz4_feature gated under cfg(not(feature = "lz4-compression")) covers both the reject and the 0-threshold accept paths. 2. Read-stub error variant (Bug) decompress_block on the feature-off stub returned Error::Corruption, which operators read as disk damage and triggers the repair flow. The data isn't corrupt - this build cannot decode it. Switched to Error::Other with a message that names the feature flag, matching the write-path stub. 3. Deduplicate DEFAULT_LZ4_COMPRESSION_LEVEL (Nit) Hoisted the constant to parent scope in util.rs; both lz4 modules now pub use super::DEFAULT_LZ4_COMPRESSION_LEVEL. Eliminates drift risk. 4. Document on-disk compat in Config rustdoc (Risk) The rustdoc on batch_compression_threshold now calls out the feature-off constraint (must be 0 / cannot decode feature-on data) so callers don't need to read the changelog to discover it. 5. CHANGELOG + README feature entry (Risk) Added an [Unreleased] bullet and a new entry under "Available Cargo features" in the README explaining what the feature does, what disabling costs, and the forward-compat constraint. 6. CI coverage for --no-default-features (Risk) Added a cargo build --no-default-features --verbose step to the stable job in .github/workflows/rust.yml, right after clippy. Without this, the next unrelated PR that adds a use crate::util::lz4::X reference outside the real module would silently regress the feature-off build. Verification: cargo check --lib --no-default-features and cargo check --lib both pass (0 errors). Same two-arm cargo tree grep check as the prior commit - lz4-sys absent without the feature, present with it. The [patch.crates-io] cc dead-patch warning on feature-off builds noted in the review is left as a separate concern; the pin is a workaround for a cc-rs issue whose applicability in 2026 warrants a separate PR, not a bundled change here. Signed-off-by: Archith <archith.rapaka@gmail.com>
CodeRabbit review on PR tikv#397 flagged that the new 'Build (no default features)' CI step ran only 'cargo build', which neither compiles nor runs the test_sanitize_rejects_nonzero_threshold_without_lz4_feature test in src/config.rs (gated #[cfg(not(feature = "lz4-compression"))]). The test silently never ran in any CI job, defeating the point of adding it. Replaces the step with: cargo check --no-default-features --all-targets --verbose cargo test --no-default-features --verbose --all-targets covers tests/benches/examples in the no-default-features graph; the test invocation actually exercises the gated reject + accept paths in Config::sanitize. Signed-off-by: Archith <archith.rapaka@gmail.com>
humancto
force-pushed
the
feat/feature-gate-lz4-sys
branch
from
ea4e85c to
4e01f4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 89-94: The doc comment in the lz4-compression feature block refers
to the wrong config key name ("compression_threshold = 0"); update the comment
to reference the actual TOML/serde key "batch-compression-threshold = 0"
(matching #[serde(rename_all = "kebab-case")]) so examples in the Cargo.toml doc
are correct and users won't copy an incorrect field name.
In `@src/config.rs`:
- Around line 216-221: Config::default() currently sets
batch_compression_threshold to ReadableSize::kb(8) which becomes the serde
default and causes sanitize() to error when the lz4-compression feature is
disabled; modify Config::default() to set batch_compression_threshold to zero
when the lz4 feature is not enabled (use the same conditional pattern already
applied to memory_limit in Config::default()), so deserialising TOML that omits
batch-compression-threshold yields a valid default; ensure you reference the
field name batch_compression_threshold and keep the ReadableSize type (set to
zero) and leave the sanitize() behavior unchanged.
🪄 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
Run ID: e27ebc26-718e-4764-af14-f6c2009731d4
📒 Files selected for processing (6)
.github/workflows/rust.ymlCHANGELOG.mdCargo.tomlREADME.mdsrc/config.rssrc/util.rs
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/rust.yml
- CHANGELOG.md
- README.md
- src/util.rs
Comment on lines
+89
to
+94
| # Optional LZ4 compression for log batches via the `lz4-sys` C FFI crate. | ||
| # Default-on preserves historical behavior; disabling it removes the C | ||
| # dependency entirely from the build graph. Callers who disable this | ||
| # feature must set `compression_threshold = 0` in their `Config`, and | ||
| # must not open existing data written with compression enabled. | ||
| lz4-compression = ["dep:lz4-sys"] |
There was a problem hiding this comment.
Doc comment uses the wrong field name: compression_threshold → should be batch-compression-threshold.
Line 92 references compression_threshold = 0, but the actual TOML key is batch-compression-threshold (due to #[serde(rename_all = "kebab-case")]). Users copying from this comment will either hit a silent no-op key or a deserialization error depending on their strictness settings.
📝 Proposed fix
-# feature must set `compression_threshold = 0` in their `Config`, and
+# feature must set `batch-compression-threshold = 0` in their `Config`, and📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Optional LZ4 compression for log batches via the `lz4-sys` C FFI crate. | |
| # Default-on preserves historical behavior; disabling it removes the C | |
| # dependency entirely from the build graph. Callers who disable this | |
| # feature must set `compression_threshold = 0` in their `Config`, and | |
| # must not open existing data written with compression enabled. | |
| lz4-compression = ["dep:lz4-sys"] | |
| # Optional LZ4 compression for log batches via the `lz4-sys` C FFI crate. | |
| # Default-on preserves historical behavior; disabling it removes the C | |
| # dependency entirely from the build graph. Callers who disable this | |
| # feature must set `batch-compression-threshold = 0` in their `Config`, and | |
| # must not open existing data written with compression enabled. | |
| lz4-compression = ["dep:lz4-sys"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` around lines 89 - 94, The doc comment in the lz4-compression
feature block refers to the wrong config key name ("compression_threshold = 0");
update the comment to reference the actual TOML/serde key
"batch-compression-threshold = 0" (matching #[serde(rename_all = "kebab-case")])
so examples in the Cargo.toml doc are correct and users won't copy an incorrect
field name.
Comment on lines
+216
to
+221
| #[cfg(not(feature = "lz4-compression"))] | ||
| if self.batch_compression_threshold.0 > 0 { | ||
| return Err(box_err!( | ||
| "batch-compression-threshold must be 0 when the `lz4-compression` Cargo feature is disabled" | ||
| )); | ||
| } |
There was a problem hiding this comment.
Config::default() produces an invalid config when lz4-compression is disabled, breaking four existing tests under --no-default-features.
Config::default() always sets batch_compression_threshold: ReadableSize::kb(8). Because #[serde(default)] on the struct uses the Default implementation for missing fields, if the value is not present when deserializing, Default::default() is used — so any TOML config that omits batch-compression-threshold will silently inherit 8 KB. With the new sanitize() guard in place, this causes every existing test that (a) deserialises TOML without that key and (b) calls sanitize() to panic under --no-default-features:
| Test | Failing line | Why |
|---|---|---|
test_custom |
302 .unwrap() |
TOML omits the field → default 8 KB → Err |
test_invalid (soft case) |
326 .unwrap() |
TOML omits the field → default 8 KB → Err |
test_backward_compactibility |
367 .unwrap() |
TOML omits the field → default 8 KB → Err |
test_prefill_for_recycle |
381, 390 .is_ok() |
TOML omits the field → default 8 KB → Err |
The newly added CI step (cargo test --no-default-features) will immediately fail on all four.
The clean fix is to zero out batch_compression_threshold inside Config::default() under the same pattern already used for memory_limit in test mode (lines 155–159), which would also make Config::default() produce a valid config for library consumers who build without lz4-compression:
🐛 Proposed fix in Config::default() (lines 133–162)
// Test-specific configurations.
#[cfg(test)]
{
cfg.memory_limit = Some(ReadableSize(0));
}
+ // When lz4-compression is disabled, force a valid default so that
+ // Config::default() does not immediately fail sanitize().
+ #[cfg(not(feature = "lz4-compression"))]
+ {
+ cfg.batch_compression_threshold = ReadableSize(0);
+ }
cfgThe new test_sanitize_rejects_nonzero_threshold_without_lz4_feature test (lines 350–358) explicitly overrides the threshold to ReadableSize::kb(8) before the first assert!, so it remains correct after this fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.rs` around lines 216 - 221, Config::default() currently sets
batch_compression_threshold to ReadableSize::kb(8) which becomes the serde
default and causes sanitize() to error when the lz4-compression feature is
disabled; modify Config::default() to set batch_compression_threshold to zero
when the lz4 feature is not enabled (use the same conditional pattern already
applied to memory_limit in Config::default()), so deserialising TOML that omits
batch-compression-threshold yields a valid default; ensure you reference the
field name batch_compression_threshold and keep the ReadableSize type (set to
zero) and leave the sanitize() behavior unchanged.
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
2 participants
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.
Summary
Make the
lz4-sysC FFI crate optional behind a newlz4-compressionCargo feature (default-on). Consumers who build withdefault-features = falseand withoutlz4-compressionget a pure-Rust build graph withlz4-sysfully absent; existing consumers on defaults see zero behavior change.Motivation
Projects with a strict pure-Rust policy (no C dependencies in the build graph) currently cannot consume
raft-engineat all becauselz4-sys = "=1.9.5"is unconditional. Making the C dep opt-in-by-default removes that adoption blocker without affecting anyone who stays on defaults.Changes
Cargo.toml:lz4-sysmarkedoptional = true; new featurelz4-compression = ["dep:lz4-sys"];default = ["internals", "scripting", "lz4-compression"]preserves existing BC.src/util.rs:pub mod lz4split into two#[cfg]-gated variants with identical public surface (DEFAULT_LZ4_COMPRESSION_LEVEL,append_compress_block,decompress_block). Real implementation behind#[cfg(feature = "lz4-compression")]. Stub under#[cfg(not(...))]returnsError::Otherwith a message naming the feature flag. Call sites insrc/log_batch.rscompile unchanged under both arms.src/config.rs:Config::sanitizerejectsbatch_compression_threshold > 0when the feature is off, matching the existing reject patterns forenable_log_recycle/prefill_for_recycle. Rustdoc onbatch_compression_thresholdspells out the constraint. New unit test covers both reject and accept paths.CHANGELOG.md+README.md: entry under[Unreleased]and a line in the "Available Cargo features" list..github/workflows/rust.yml: newcargo build --no-default-features --verbosestep in the stable job to regression-guard the feature gate.Operator contract
Callers who disable the feature:
batch-compression-threshold = 0inConfig(enforced bysanitize).Error::Otheron read, naming the feature flag.Test plan
cargo check --libpasses (0 errors).cargo check --lib --no-default-featurespasses (0 errors).cargo tree --no-default-features --edges no-dev | grep lz4→ empty.cargo tree --edges no-dev | grep lz4→lz4-sys v1.9.5still present (defaults unchanged).--no-default-featuresstep.Pre-existing warning flagged in review
The new CI step will surface a pre-existing
warning: Patch cc v1.0.98 ... was not used in the crate graph.This is the[patch.crates-io] cc = { ... tag = "1.0.98" }pin added for rust-lang/cc-rs#984, which no longer matches the resolvedcc 1.2.61in the lockfile. The warning is orthogonal to this change and predates it — left as a follow-up since the original #984 workaround should be re-evaluated on its own merits rather than bundled into a feature-gate patch.Background
Part of the humancto/mango project's adoption of
raft-engineas its WAL. Tracking the fork's retirement against this upstream PR's merge.Summary by CodeRabbit
New Features
Documentation
Chores
Tests