Skip to content

Commit

Permalink
kv: don't load applied index twice during Raft snapshot
Browse files Browse the repository at this point in the history
This was harmless, but it was also unnecessary and looked quite
alarming. It would be a major problem if the Raft applied index stored
in a `SnapshotMetadata` was ever incoherent with the applied state in
the snapshot. However, we're reading from a consistent enging snapshot
in `snapshot`, so the Raft applied index can't change. Still, better to
avoid questions.

The "members of the Range struct" comment was very stale, dating back
to acc41dd.
  • Loading branch information
nvanbenschoten authored and Sajjad Rizvi committed Aug 10, 2021
1 parent 722172e commit 7d6353e
Showing 1 changed file with 4 additions and 11 deletions.
15 changes: 4 additions & 11 deletions pkg/kv/kvserver/replica_raftstorage.go
Original file line number Diff line number Diff line change
Expand Up @@ -575,21 +575,14 @@ func snapshot(
return OutgoingSnapshot{}, errors.Mark(errors.Errorf("couldn't find range descriptor"), errMarkSnapshotError)
}

// Read the range metadata from the snapshot instead of the members
// of the Range struct because they might be changed concurrently.
appliedIndex, _, err := rsl.LoadAppliedIndex(ctx, snap)
state, err := rsl.Load(ctx, snap, &desc)
if err != nil {
return OutgoingSnapshot{}, err
}

term, err := term(ctx, rsl, snap, rangeID, eCache, appliedIndex)
if err != nil {
return OutgoingSnapshot{}, errors.Errorf("failed to fetch term of %d: %s", appliedIndex, err)
}

state, err := rsl.Load(ctx, snap, &desc)
term, err := term(ctx, rsl, snap, rangeID, eCache, state.RaftAppliedIndex)
if err != nil {
return OutgoingSnapshot{}, err
return OutgoingSnapshot{}, errors.Errorf("failed to fetch term of %d: %s", state.RaftAppliedIndex, err)
}

// Intentionally let this iterator and the snapshot escape so that the
Expand All @@ -606,7 +599,7 @@ func snapshot(
RaftSnap: raftpb.Snapshot{
Data: snapUUID.GetBytes(),
Metadata: raftpb.SnapshotMetadata{
Index: appliedIndex,
Index: state.RaftAppliedIndex,
Term: term,
// Synthesize our raftpb.ConfState from desc.
ConfState: desc.Replicas().ConfState(),
Expand Down

0 comments on commit 7d6353e

Please sign in to comment.