numinnex commented on PR #3767: URL: https://github.com/apache/iggy/pull/3767#issuecomment-5116903237
Reviewed this in the context of the upcoming metadata state transfer work, which will build on top of it. The design looks sound overall; two issues I would consider blockers. ### 1. WAL scan refuses boot on any pre-existing WAL (`prepare_journal.rs`, scan) The `checksum_body` verification has no format gate. Every entry written before this change carries `checksum_body == 0`, so the scan fails all of them, and the interior-corruption branch refuses boot. It also breaks mixed-version clusters: an old-version primary sends zero-checksum prepares, a new-version backup journals them verbatim, and that backup's next restart refuses boot, so a rolling upgrade is unsafe in that direction. The `TODO(wal-integrity)` already names this, but I think it needs to be part of this PR rather than a follow-up: treat `checksum_body == 0` as unsealed and skip verification for that entry, or gate on a WAL entry format version. ### 2. `combine_slots` silently regresses durable state on newest-slot bit-rot (`superblock.rs`) `(Valid, Corrupt)` falls back to the older valid record. A torn write dies in the `.tmp` (rename is atomic), so a corrupt non-temp slot can only be bit-rot. When the bit-rot hits the newest slot, recovery boots from the older generation: older `view`/`log_view`, and the replica can re-vote in a view it already acted in, which is exactly the split-brain this PR exists to prevent. Since the corrupt slot's sequence is unreadable, there is no way to tell whether it was the newer one, so I believe the sound response for `Valid + Corrupt` is refusing boot, the same as both-corrupt. There is a compounding issue in `PingPongSuperblock::open`: `read_sequence` maps both Absent and Corrupt to `None`, so the next write targets the corrupt (possibly newest) slot and overwrites the evidence with `sequence = valid + 1`, which retroactively legitimizes the regression. Distinguishing Absent from Corrupt there would fall out of the same fix. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
