test(mem_wal): cover cross-generation durable-put fencing end to end#7900
test(mem_wal): cover cross-generation durable-put fencing end to end#7900u70b3 wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds and registers a Tokio integration test covering durable MemWAL writes across MemTable generation rotation. The test verifies that a higher-epoch peer fences the subsequent write and returns the expected fencing reason and error message. ChangesMemWAL durability fencing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
rust/lance/tests/mem_wal.rs-66-66 (1)
66-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd diagnostic messages to the bare assertions.
When these checks fail, the test reports only
assertion failed, hiding the epochs and returned error that explain the fencing regression. As per coding guidelines, assertions must include descriptive messages.Proposed improvement
- assert!(writer_b.epoch() > writer_a.epoch()); + assert!( + writer_b.epoch() > writer_a.epoch(), + "writer B epoch must exceed writer A epoch: A={}, B={}", + writer_a.epoch(), + writer_b.epoch() + ); - assert!(error.to_string().contains("Writer fenced")); + assert!( + error.to_string().contains("Writer fenced"), + "expected a writer-fenced error, got: {error}" + );Also applies to: 78-78
🤖 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/tests/mem_wal.rs` at line 66, Update the bare assertions comparing writer epochs in the mem_wal test, including the corresponding assertion at the other referenced location, to include descriptive diagnostic messages that report the relevant epoch values and returned error details when available.Source: Coding guidelines
rust/lance/tests/mem_wal.rs-25-27 (1)
25-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the existing path-to-URI conversion here.
format!("file://{}", temp_dir.path().display())is brittle on Windows and skips URI escaping; build thefile:URL withUrl::from_directory_path(temp_dir.path())before passing it toObjectStore::from_uri.🤖 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/tests/mem_wal.rs` around lines 25 - 27, Replace the manual base_uri formatting in the mem_wal test with the existing Url::from_directory_path(temp_dir.path()) conversion, handling its result as required, then pass the resulting file URL to ObjectStore::from_uri. Keep the temporary directory and store initialization flow unchanged.
🤖 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/tests/mem_wal.rs`:
- Line 66: Update the bare assertions comparing writer epochs in the mem_wal
test, including the corresponding assertion at the other referenced location, to
include descriptive diagnostic messages that report the relevant epoch values
and returned error details when available.
- Around line 25-27: Replace the manual base_uri formatting in the mem_wal test
with the existing Url::from_directory_path(temp_dir.path()) conversion, handling
its result as required, then pass the resulting file URL to
ObjectStore::from_uri. Keep the temporary directory and store initialization
flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 69dc67e6-7ec6-42b7-b93a-21024ee590d5
📒 Files selected for processing (2)
rust/lance/tests/integration_tests.rsrust/lance/tests/mem_wal.rs
|
@hamersaw could you take a look when you have a moment? This ports the public-surface regression test from the closed #7759 onto your #7888 cursor design — the sequence it pins down is the same one your lib-level |
54a7d85 to
020b146
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance/tests/mem_wal.rs (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
memory://here.
This test only exercises MemWAL fencing, so the temporaryfile://setup is unnecessary boilerplate. Switch toObjectStore::from_uri("memory://")unless it specifically needs filesystem-backed behavior.Suggested fix
- let temp_dir = tempfile::tempdir().unwrap(); - let base_uri = format!("file://{}", temp_dir.path().display()); + let base_uri = "memory://".to_string();🤖 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/tests/mem_wal.rs` around lines 25 - 27, Replace the temporary directory and file-based URI setup in the MemWAL test with an in-memory ObjectStore created via ObjectStore::from_uri("memory://"). Remove the unnecessary temp_dir and base_uri setup while preserving the returned store and base_path usage.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.
Nitpick comments:
In `@rust/lance/tests/mem_wal.rs`:
- Around line 25-27: Replace the temporary directory and file-based URI setup in
the MemWAL test with an in-memory ObjectStore created via
ObjectStore::from_uri("memory://"). Remove the unnecessary temp_dir and base_uri
setup while preserving the returned store and base_path usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 12fdec04-fa87-4d23-8782-c8ce49a8d89a
📒 Files selected for processing (2)
rust/lance/tests/integration_tests.rsrust/lance/tests/mem_wal.rs
020b146 to
4dd337d
Compare
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/tests/mem_wal.rs-57-60 (1)
57-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd diagnostic messages to the assertions.
When generation or fencing behavior regresses, these assertions currently provide little context about the expected state.
As per coding guidelines, assertions should always include descriptive messages.
Proposed change
assert_eq!( writer_a.memtable_stats().await.unwrap().generation, - first_generation + 1 + first_generation + 1, + "expected rotation from generation {first_generation}" ); - assert!(writer_b.epoch() > writer_a.epoch()); + assert!( + writer_b.epoch() > writer_a.epoch(), + "peer epoch must exceed writer A epoch" + ); - assert_eq!(error.fence_reason(), Some(FenceReason::PeerClaimedEpoch)); - assert!(error.to_string().contains("Writer fenced")); + assert_eq!( + error.fence_reason(), + Some(FenceReason::PeerClaimedEpoch), + "expected a peer-claimed-epoch fence" + ); + assert!( + error.to_string().contains("Writer fenced"), + "expected the legacy fence prefix in error: {error}" + );Also applies to: 66-78
🤖 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/tests/mem_wal.rs` around lines 57 - 60, Add descriptive failure messages to the generation and fencing assertions in the memtable WAL test, including the assertions around writer_a.memtable_stats() and the related checks at the referenced nearby locations. State the expected generation or fencing behavior in each message while preserving the existing assertion conditions.Source: Coding guidelines
🧹 Nitpick comments (1)
rust/lance/tests/mem_wal.rs (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the in-memory object store for this test.
This test does not exercise filesystem behavior, so the temporary directory adds unnecessary disk I/O and platform-dependent URI formatting. The two writers already share the same
store, somemory://should preserve the required state.As per coding guidelines, tests should use plain
"memory://"URIs; no atomic counters or unique suffixes are needed.Proposed change
- let temp_dir = tempfile::tempdir().unwrap(); - let base_uri = format!("file://{}", temp_dir.path().display()); + let base_uri = "memory://".to_string();🤖 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/tests/mem_wal.rs` around lines 25 - 27, Update the test setup around ObjectStore::from_uri to use the plain "memory://" URI instead of creating a tempfile and formatting a file URI; preserve the shared store used by both writers and remove the now-unneeded temporary-directory setup.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/tests/mem_wal.rs`:
- Around line 57-60: Add descriptive failure messages to the generation and
fencing assertions in the memtable WAL test, including the assertions around
writer_a.memtable_stats() and the related checks at the referenced nearby
locations. State the expected generation or fencing behavior in each message
while preserving the existing assertion conditions.
---
Nitpick comments:
In `@rust/lance/tests/mem_wal.rs`:
- Around line 25-27: Update the test setup around ObjectStore::from_uri to use
the plain "memory://" URI instead of creating a tempfile and formatting a file
URI; preserve the shared store used by both writers and remove the now-unneeded
temporary-directory setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 96d08847-b58d-4afe-a922-f43d965c6207
📒 Files selected for processing (2)
rust/lance/tests/integration_tests.rsrust/lance/tests/mem_wal.rs
4dd337d to
147eab4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance/tests/mem_wal.rs (1)
66-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd diagnostic messages to the bare assertions.
The
assert!calls at Line 66 and Line 78 provide no failure context. Include the actual epochs and error text so setup or fencing regressions are immediately diagnosable.Suggested change
- assert!(writer_b.epoch() > writer_a.epoch()); + assert!( + writer_b.epoch() > writer_a.epoch(), + "expected peer epoch to increase: writer_b={}, writer_a={}", + writer_b.epoch(), + writer_a.epoch() + ); ... - assert!(error.to_string().contains("Writer fenced")); + assert!( + error.to_string().contains("Writer fenced"), + "unexpected fence error: {}", + error + );As per coding guidelines, assertions must include descriptive messages.
Also applies to: 78-78
🤖 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/tests/mem_wal.rs` at line 66, Update the bare assertions in the mem_wal test at the epoch comparisons around writer_a and writer_b to include descriptive failure messages containing both actual epoch values. Apply the same diagnostic-message requirement to the assertion at the referenced second location, including the relevant error text and runtime values available there.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.
Nitpick comments:
In `@rust/lance/tests/mem_wal.rs`:
- Line 66: Update the bare assertions in the mem_wal test at the epoch
comparisons around writer_a and writer_b to include descriptive failure messages
containing both actual epoch values. Apply the same diagnostic-message
requirement to the assertion at the referenced second location, including the
relevant error text and runtime values available there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 348da59e-0a13-4453-a369-893f429f75ae
📒 Files selected for processing (2)
rust/lance/tests/integration_tests.rsrust/lance/tests/mem_wal.rs
Batch positions restart at 0 in every MemTable generation, so a durability watch keyed on the generation-local position can be satisfied by the previous generation's flush at the same position (lance-format#7760). The lib-level regression test_durable_ack_after_rotation_requires_its_own_wal_append covers the false acknowledgement itself; add the public-surface counterpart from the closed lance-format#7759: after a rotation, a peer claims a higher epoch, and writer A's first durable put into the new generation must wait for its own WAL flush and therefore surface the PeerClaimedEpoch fence instead of acking from the stale generation's durability state.
68821ac to
927611d
Compare
1 participant
Summary
Ports the public-surface regression test from the closed #7759 onto the post-#7888 design.
ShardWriterAPI: after a MemTable rotation, a peer writer claims a higher epoch, and the first durable put into the new generation must wait for its own WAL flush and surface thePeerClaimedEpochfence — not ack from the stale generation's durability state.test_durable_ack_after_rotation_requires_its_own_wal_appendadded in fix(mem_wal): read-your-writes via split index-apply and dual visibility cursors #7888, which covers the false acknowledgement (bug: durable MemTable writes can be acknowledged by an older generation #7760) via internal stats; this test exercises the same failure sequence purely through the public API (open/put/force_seal_active/wait_for_flush_drain) and asserts the typedFenceReason.Background
Batch positions restart at 0 in every MemTable generation. Before the writer-global durability cursor (#7888), a durability watch keyed on the generation-local position could be satisfied by the previous generation's flush at the same position — the first durable put after a rotation returned success before its own WAL append completed. The sequence here (rotate → peer claims epoch → durable put at the same local position) makes that false success observable as a missing
PeerClaimedEpocherror.Pre-#7888 failure sequence (what the test must never allow again)
sequenceDiagram autonumber participant C as Client participant G0 as Writer A / generation N participant M as Shared durability watermark participant G1 as Writer A / generation N+1 participant B as Writer B participant W as WAL C->>G0: durable put, local range 0..1 G0->>W: flush generation N W-->>G0: success G0->>M: publish watermark 1 Note over G0,G1: MemTable rotates and local positions restart at 0 B->>W: claim a higher writer epoch C->>G1: durable put, local range 0..1 G1->>M: wait for watermark >= 1 M-->>G1: already satisfied by generation N G1-->>C: incorrect success G1->>W: flush generation N+1 W-->>G1: PeerClaimedEpochSteps 1–4 map to the first
putplusforce_seal_active+wait_for_flush_drain, step 6 to opening writer B, and steps 7–13 to the secondput.Post-#7888 behavior (what the test asserts)
The durability cursor is writer-global and the put's target is lifted through
BatchStore::global_offset(), so the wait can only be satisfied by an append covering this generation's own range — and the peer fence arrives before any success can be reported:sequenceDiagram autonumber participant C as Client participant G1 as Writer A / generation N+1 participant Cur as WriterCursors (writer-global) participant H as WalFlushHandler participant W as WAL C->>G1: durable put, local range 0..1 G1->>G1: durable target = global_offset(1) + 1 = 2 G1->>H: trigger flush of generation N+1 G1->>Cur: wait until durable() >= 2 (cursor at 1) H->>W: append generation N+1 range W-->>H: PeerClaimedEpoch (writer B claimed the epoch) H->>Cur: latch terminal error, wake waiters Cur-->>C: Error::Fenced(PeerClaimedEpoch)The test asserts exactly step 8:
error.fence_reason() == Some(FenceReason::PeerClaimedEpoch).Validation
cargo test -p lance --test integration_tests mem_wal::durable_put_does_not_alias_across_memtable_generations -- --exact— passed (1/1)cargo fmt --all -- --check— passedcargo clippy --all --tests --benches -- -D warnings— passedCompatibility
Test-only change; no API, format, or dependency changes.