fix(mem-wal): reject mismatched shard specs before epoch claim#7949
fix(mem-wal): reject mismatched shard specs before epoch claim#7949u70b3 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesShard claim validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance/src/dataset/mem_wal/manifest.rs-445-447 (1)
445-447: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument sealed-manifest refusal in
# Errors.
claim_epochnow returns an error for sealed manifests, but this public API’s error contract only documents epoch conflicts, shard-spec conflicts, and contention. Document the sealed refusal and its no-claim effect. As per coding guidelines, “Ensure doc comments match actual semantics; distinguish mutates-in-place (&mut self) from returns-new-value behavior.”🤖 Prompt for 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. In `@rust/lance/src/dataset/mem_wal/manifest.rs` around lines 445 - 447, Update the public `claim_epoch` documentation comment’s `# Errors` section to include refusal for sealed manifests and state that this path makes no claim or mutation. Keep the existing documentation for epoch conflicts, shard-spec conflicts, and contention, and accurately describe the method’s in-place `&mut self` behavior.Source: Coding guidelines
🤖 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.
Other comments:
In `@rust/lance/src/dataset/mem_wal/manifest.rs`:
- Around line 445-447: Update the public `claim_epoch` documentation comment’s
`# Errors` section to include refusal for sealed manifests and state that this
path makes no claim or mutation. Keep the existing documentation for epoch
conflicts, shard-spec conflicts, and contention, and accurately describe the
method’s in-place `&mut self` behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: eddae87c-eea7-4d35-949b-1203ad196d84
📒 Files selected for processing (1)
rust/lance/src/dataset/mem_wal/manifest.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Summary
shard_spec_idmismatches before advancing a shardmanifest version or writer epoch
distinguishable sealed-shard error
rejection, and concurrent claims with different specs
flowchart TD A["Read latest manifest"] B{"Sealed?"} C{"Spec identity matches?"} D["Return sealed error"] E["Return InvalidInput<br/>manifest unchanged"] F["Write next version / epoch"] G{"Write conflict?"} A --> B B -- Yes --> D B -- No --> C C -- No --> E C -- Yes --> F F --> G G -- Yes --> Ashard_spec_id=0remains the immutable identity for manually created shards;it is not treated as a wildcard.
Fixes #7945.
Test Plan
cargo fmt --all -- --checkcargo test -p lance dataset::mem_wal::manifest::tests --lib -- --nocapturecargo clippy --all --tests --benches -- -D warnings