kv: don't load applied index twice during Raft snapshot #68497
Conversation
|
This change is |
There was a problem hiding this comment.
Getting that twice looks unnecessary.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)
pkg/kv/kvserver/replica_raftstorage.go, line 553 at r1 (raw file):
Isn't that contradicting that you said in the PR comment
"However, we're holding the raftMu in snapshot, so the Raft applied index can't change."
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.
nvanbenschoten
force-pushed
the
nvanbenschoten/appliedIndexLoad
branch
from
059d451
to
60ce7be
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_raftstorage.go, line 553 at r1 (raw file):
Previously, aliher1911 (Oleg) wrote…
Isn't that contradicting that you said in the PR comment
"However, we're holding the raftMu in snapshot, so the Raft applied index can't change."
Yeah, you're completely right. I was missing the fact that we're reading from an engine snapshot. Fixed the commit message.
|
bors r+ |
|
Build succeeded: |
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
SnapshotMetadatawas ever incoherent with the applied state inthe snapshot. However, we're reading from a consistent enging snapshot
in
snapshot, so the Raft applied index can't change. Still, better toavoid questions.
The "members of the Range struct" comment was very stale, dating back
to acc41dd.