krishvishal commented on code in PR #3824:
URL: https://github.com/apache/iggy/pull/3824#discussion_r3734783913
##########
core/consensus/src/impls.rs:
##########
@@ -3062,15 +3485,25 @@ where
// sealed region before the entry is journaled. Leaving those prepares
at `0`
// is the designed "nothing to verify" sentinel, so a future durable
partition
// journal skips verification instead of failing every entry as
corrupt.
+ //
+ // So a partition prepare's `checksum` identifies its header alone,
and two
+ // such prepares at one op with matching header fields are
indistinguishable
+ // to the view-change merge. Closing that wants `checksum_body` here
to BE
+ // the batch checksum, recomputed after stamping, so `checksum` covers
the
+ // body for free by hashing this field.
+ //
+ // Bounded by `size`, the range every verifier re-reads; the prepare
+ // inherits it verbatim below.
let checksum_body = if consensus.namespace ==
METADATA_CONSENSUS_NAMESPACE {
Review Comment:
Not fixing this in the review pass, deliberately. The comment is now a TODO
spelling out the merge consequence you named, plus both closures with their
hazards:
1. Recomputing after `stamp_prepare_for_persistence` changes the identity at
stamp time and folds in `base_offset`, desyncing the pipeline entry from the
journaled one that `handle_prepare_ok` compares.
2. Hashing the stamp-invariant cover (everything past the command header) is
safe here, but costs a produce-path pass and retires the sentinel that lets
existing WALs replay.
Which one to take is a maintainer call, not a review 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]
