Skip to content

Commit

Permalink
Merge #131503
Browse files Browse the repository at this point in the history
131503: apply: fix initial snapshot assertion r=arulajmani a=pav-kv

The broad assertion here is that all the entries composing a snapshot must have been applied by someone before (at least the sender of the snapshot has done it, otherwise it couldn’t have sent this snapshot).

And then there is an additional assertion (which failed occasionally before this PR) that there can’t be a snapshot without at least one entry applied. This is generally true, except that there are entries not registered by the "apply" stack: particularly, empty entries that raft leader appends at the start of its term. There is a comment to this extent in the `asserter.go` code:
 https://github.com/cockroachdb/cockroach/blob/5b0371b44009ac2ae3f58ea7ed95b290dd4e8227/pkg/kv/kvserver/apply/asserter.go#L254-L259

This commit modifies the snapshot assertion to correctly handle this case. It is possible that an initial snapshot sent from leader to followers contains only this dummy entry, and there were no "real" proposals applied.

Fixes the false assertion failure in #118471 (comment)
Related to #116319

Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
  • Loading branch information
craig[bot] and pav-kv committed Sep 27, 2024
2 parents 2d531bd + 5aeac1a commit 9d3d6d9
Showing 1 changed file with 11 additions and 5 deletions.
16 changes: 11 additions & 5 deletions pkg/kv/kvserver/apply/asserter.go
Original file line number Diff line number Diff line change
Expand Up @@ -380,17 +380,23 @@ func (r *rangeAsserter) applySnapshot(
r.Lock()
defer r.Unlock()

// INVARIANT: we can't have a snapshot without any applied log entries.
if len(r.log) == 0 {
fail("snapshot before any log entries applied")
}

// INVARIANT: a snapshot must progress the replica's applied index.
if ri := r.replicaAppliedIndex[replicaID]; index <= ri {
fail("replica applied index regression: %d -> %d", ri, index)
}
r.replicaAppliedIndex[replicaID] = index

// We can't have a snapshot without any applied log entries, except when this
// is an initial snapshot. It's possible that the initial snapshot follows an
// empty entry appended by the raft leader at the start of this term. Since we
// don't register this entry as applied, r.log can be empty here.
//
// See the comment in r.apply() method, around the empty cmdID check, and the
// comment for r.log saying that there can be gaps in the observed applies.
if len(r.log) == 0 {
return
}

// INVARIANT: a snapshot must match the applied state at the given Raft index.
//
// The snapshot may point beyond the log or to a gap in our log because of
Expand Down

0 comments on commit 9d3d6d9

Please sign in to comment.