CraftReplDev write path#165
Conversation
There was a problem hiding this comment.
Pull request overview
Implements the client-facing write path for CraftReplDev (CRAFT epic S2), introducing a new volume_error::STALE_TERM for session/term rejection and adding initial unit tests plus a stub production journal backend factory.
Changes:
- Add
CraftReplDev::write()implementation with term checking and missing-LSN tracking. - Introduce
HomeStoreCraftJournalBackendstub +make_homestore_journal_backend()factory for production wiring. - Add
CraftWriteTestwith an in-memory journal backend and CMake integration for craft tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/craft/tests/test_craft_write.cpp | Adds unit tests and an in-memory MockCraftJournalBackend to validate write ordering and term rejection behavior. |
| src/lib/craft/tests/CMakeLists.txt | Adds test_craft_write target and registers it with CTest. |
| src/lib/craft/craft_repl_dev.hpp | Adds missing-set observability helpers, makes the mutex mutable, and declares the HomeStore journal backend factory. |
| src/lib/craft/craft_repl_dev.cpp | Adds the production backend stub + factory and replaces the write() stub with a real implementation. |
| src/lib/craft/CMakeLists.txt | Adds tests/ subdirectory to the craft build. |
| src/include/homeblks/home_blocks.hpp | Extends volume_error with STALE_TERM. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 2. Update missing set and last_append_lsn under lock. | ||
| // Done before the journal write so the missing set is always conservative: | ||
| // a failed write leaves the LSN in the set (correctly marked absent). | ||
| { | ||
| std::lock_guard lock{missing_mu_}; | ||
| // Any LSNs between the old last_append and this one are gaps. | ||
| for (int64_t gap = state_.last_append_lsn + 1; gap < lsn; ++gap) { | ||
| missing_lsns_.insert(gap); | ||
| } | ||
| // This slot is being filled. | ||
| missing_lsns_.erase(lsn); | ||
| state_.last_append_lsn = std::max(state_.last_append_lsn, lsn); | ||
| } | ||
|
|
||
| // 3. Journal append — zero-copy; data buffer is not copied on the hot path. | ||
| auto res = co_await journal_->write_slot(lsn, lba, len, std::move(data)); | ||
| if (!res) { | ||
| LOGE("write_slot failed lsn={} lba={} len={}: {}", lsn, lba, len, res.error().message()); | ||
| co_return std::unexpected(res.error()); | ||
| } | ||
|
|
||
| LOGT("write ok lsn={} lba={} len={}", lsn, lba, len); | ||
| co_return homestore::ok(); |
| // 1. Term check — old-session client; must re-login. | ||
| if (term != state_.term) { | ||
| LOGW("write rejected: stale term want={} got={} lsn={}", state_.term, term, lsn); | ||
| co_return std::unexpected(make_error_condition(volume_error::STALE_TERM)); | ||
| } |
| ENUM(volume_error, uint16_t, UNKNOWN_VOLUME = 1, CRC_MISMATCH, INDEX_ERROR, INTERNAL_ERROR, OFFLINE, | ||
| STALE_TERM | ||
| ); |
| // Term rejection: write with term != state_.term returns STALE_TERM. | ||
| // Journal must not be touched. | ||
| TEST_F(CraftWriteTest, TermRejection) { | ||
| // state_.term starts at 0; term=1 is stale. |
18dc68d to
fd58967
Compare
| // ─── HomeStoreCraftJournalBackend ───────────────────────────────────────────── | ||
| // | ||
| // Production journal backend. Wraps the HomeStore data-journal at client-assigned |
| // Term rejection: write with term != state_.term returns STALE_TERM. | ||
| // Journal must not be touched. | ||
| TEST_F(CraftWriteTest, TermRejection) { | ||
| // state_.term starts at 0; term=1 is stale. | ||
| auto res = do_write(1 /* wrong term */, 0); |
shosseinimotlagh
left a comment
shosseinimotlagh
left a comment
There was a problem hiding this comment.
NIT: add ticket number on the pr title (for instance : SDSTOR-22385)
fd58967 to
b51b3fc
Compare
There was a problem hiding this comment.
Reviewed against the S2 acceptance criteria in docs/craft/subtasks.md and the CRAFT-on-HomeBlocks wiki. Leaving signature/return-type mismatches for the conflict resolution pass - a few design-level issues worth addressing first.
all_zeros thin write is missing (required by S2 AC)
The S2 acceptance criteria explicitly require:
write(term, lsn, lba, len, all_zeros, data)withall_zeros=trueskips allocation and the data-service write; journal a metadata-only slot{term, lsn, lba, len, all_zeros}. Both forms take a dLSN and merge by highest-per-LBA.
JournalSlot already has bool all_zeros{false}. There is no all_zeros parameter in write(), no zero-write branch, and no corresponding test. The AC explicitly calls out a test case: "a zero (all_zeros) write allocates no blocks and merges over/under a data write by dLSN." This is required in S2, not deferred to S3.
write_slot API contradicts the HS_DATA_LINKED pattern
The wiki is explicit (Journal backing):
A CRAFT write does not put the payload in the journal: blocks are allocated up front, the payload is written once via the data service, and the journal slot records only the reference
{term, lsn, lba, len, blkid}.
The current write_slot(lsn, lba, len, sisl::sg_list data) passes the raw payload into the backend. The production HomeStoreCraftJournalBackend cannot follow the HS_DATA_LINKED pattern with this API — the data-service call lives in CraftReplDev::write(), and only the resulting blkid should cross into the backend:
virtual async_status write_slot(int64_t lsn, lba_t lba, lba_count_t len,
homestore::MultiBlkId blkid) = 0;sisl::sg_list belongs on read_slot / fetch_data return paths (after reading blocks back from the data service), not on write_slot. The mock can keep empty blkids for now; the point is the abstraction boundary is wrong and will force a re-API when the real backend lands.
glsn parameter has no basis in the design
The S2 AC signature is write(term, lsn, lba, len, data). There is no global LSN in the protocol — CRAFT is fully dLSN-based. If this is bridging something from the wire format, it should be named and documented here. If it's speculative, drop it.
Missing test: journal write failure
The central invariant of the pre-insert scheme is that a failed write_slot leaves lsn in missing_lsns_ without corrupting future gap tracking. That is the most important property of the approach and it is untested. A MockCraftJournalBackend that can be told to fail on a specific LSN would cover this.
|
@copilot resolve the merge conflicts in this pull request |
Resolves the conflict between sbinmalek's PR eBay#165 (write path) and the current dev/v6.x API. Changes applied to a clean branch from origin/dev/v6.x: - home_blocks.hpp: add STALE_TERM to volume_error enum (server-internal counterpart to craft::craft_error::STALE_TERM on the client wire) - craft_repl_dev.hpp: add missing_count()/is_missing() test-observability helpers (const, lock mutable missing_mu_), add make_homestore_journal_backend() factory declaration - craft_repl_dev.cpp: add HomeStoreCraftJournalBackend stub (stubs for write_slot/read_slot/truncate_to with LOGW; S2 through S4 fill them), implement CraftReplDev::write() with term check, gap-tracking in missing_lsns_, lock-before-I/O pattern, lsn_pair snapshot return - craft/CMakeLists.txt: drop stale reference-model comment, wire up add_subdirectory(tests) - craft/tests/CMakeLists.txt: direct-compile test target for test_craft_write (no ${PROJECT_NAME} dep — avoids HomeStore plumbing) - craft/tests/test_craft_write.cpp: three tests — InOrderWrites, OutOfOrderWrites, TermRejection — using MockCraftJournalBackend
b51b3fc to
f44e260
Compare
- write() now takes craft::client_hdr (carries term/commit_lsn/all_committed_lsn)
and byte-addressed uint64_t addr/len instead of raw term + lba_t/lba_count_t.
Term check uses hdr.term; addr/len are cast to lba_t/lba_count_t for the
journal backend (byte-to-block division is deferred until lba_size is wired).
- read() and request_resolution() stubs updated to matching signatures.
- craft_api.cpp: dispatch through CraftReplDev* (via volume::craft_dev()) instead
of the craft::craft_replica abstract interface, which is not installed by any
version of craft_client. Matches craft_truncate exactly.
- volume.hpp: remove craft::craft_replica forward-declaration; include
craft_repl_dev.hpp directly and expose craft_dev() returning CraftReplDev*.
- test: do_write() passes craft::client_hdr{term,...} to match new signature.
f44e260 to
5d7d845
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/v6.x #165 +/- ##
===========================================
Coverage ? 45.09%
===========================================
Files ? 18
Lines ? 1020
Branches ? 442
===========================================
Hits ? 460
Misses ? 273
Partials ? 287 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Implements the client-facing write path for CraftReplDev (S2 of the CRAFT epic). Builds on the S1 foundation; the next subtask (S3) will add read, commit, and keep-alive.
Test plan