krishvishal commented on code in PR #3975:
URL: https://github.com/apache/iggy/pull/3975#discussion_r3907464714
##########
core/consensus/src/vsr_state.rs:
##########
@@ -121,16 +142,26 @@ impl TryFrom<&[u8]> for VsrState {
type Error = VsrStateError;
fn try_from(bytes: &[u8]) -> Result<Self, Self::Error> {
- // Length-tolerant: a pre-`offset_frontier` record is padded out and
the
- // new field reads as 0, which is exactly "no recorded frontier" (the
- // read sites filter it). One length check up front then puts every
- // field slice below in bounds by construction, so the `try_into`s
- // cannot fail.
+ // Length-tolerant for ONE legacy layout, the pre-`offset_reserved`
record
+ // every tagged release wrote. The one length check then puts every
field
+ // slice below in bounds by construction, so the `try_into`s cannot
fail.
+ //
+ // `offset_reserved` is filled from `offset_frontier`, not zeroed. The
two
+ // agree on a record written before the reservation existed: the
frontier
+ // is what that build's data proved, and the write side clamps the
+ // reservation up to it anyway, so this is the same value a first write
+ // under this build would record. A 0 would instead claim "nothing
+ // reserved" for offsets the frontier says exist.
+ //
+ // It cannot recover what the old build never wrote down -- offsets
acked
+ // out of RAM above the frontier are gone with the process either way
--
+ // but refusing the record recovers nothing and costs the node its
boot.
let mut padded = [0u8; ENCODED_LEN];
match bytes.len() {
ENCODED_LEN => padded.copy_from_slice(bytes),
- ENCODED_LEN_WITHOUT_FRONTIER => {
- padded[..ENCODED_LEN_WITHOUT_FRONTIER].copy_from_slice(bytes);
+ ENCODED_LEN_WITHOUT_RESERVATION => {
+
padded[..ENCODED_LEN_WITHOUT_RESERVATION].copy_from_slice(bytes);
+ padded[66..74].copy_from_slice(&bytes[58..66]);
Review Comment:
Fixed. `padded[ENCODED_LEN_WITHOUT_RESERVATION..ENCODED_LEN]`;
`bytes[58..66]` left numeric per the file.
##########
core/partitions/src/state_transfer.rs:
##########
@@ -2044,7 +2051,7 @@ where
self.reset_offset_frontier_at(offsets_wire.next_offset)
.await
} else {
- self.persist_offset_frontier_at(offsets_wire.next_offset)
+ self.install_offset_frontier_at(offsets_wire.next_offset)
Review Comment:
No, it buys nothing today, exactly as you describe. Kept and documented: the
branch is keyed on `purge_advances`, where the reset is the right shape rather
than a coincidence.
##########
core/partitions/src/iggy_partition.rs:
##########
@@ -954,7 +1124,7 @@ where
let _superblock_guard = self.superblock_lock.acquire().await;
match intended {
Some(frontier) => {
- self.write_superblock_inner(superblock.as_ref(), frontier)
+ self.write_superblock_inner(superblock.as_ref(), frontier,
frontier)
Review Comment:
Fixed. The `Some` arm writes `frontier.max(durable_offset_reserved)`, so the
monotone ceiling holds here too.
--
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]
