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 holding the raftMu in `snapshot`, so the
Raft applied index can't change. In fact, `StateLoader.Load` is only
atomic if we're holding the `raftMu` anyway. Still, better to avoid
questions.

The "members of the Range struct" comment was very stale, dating back
to acc41dd.
  • Loading branch information
nvanbenschoten committed Aug 5, 2021
1 parent 1443ced commit 059d451
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 059d451

Please sign in to comment.