-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net/raft: save snapshot position first #1312
Conversation
PTAL CoreOS confirmed this in etcd-io/etcd#8088. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one suggestion
net/raft/raft.go
Outdated
err = walobj.SaveSnapshot(walpb.Snapshot{ | ||
Index: raftSnap.Metadata.Index, | ||
Term: raftSnap.Metadata.Term, | ||
}) | ||
if err != nil { | ||
return 0, nil, errors.Wrap(err) | ||
} | ||
err := sv.saveSnapshot(&raftSnap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if sv.saveSnapshot
and wal.SaveSnapshot
could be consolidated somehow, to prevent us from accidentally doing these in the wrong order. (They always get executed together, right?)
Save the snapshot position to the WAL before saving the snapshot data to disk. On boot, we open the WAL at the most recently saved snapshot's position. If we don't save the position to the WAL first, we might open the WAL at a snapshot position that was never saved to the WAL. We might want to wait until the CoreOS folks confirm this in etcd-io/etcd#8082.
Save the snapshot position to the WAL before saving the snapshot data to
disk. On boot, we open the WAL at the most recently saved snapshot's
position. If we don't save the position to the WAL first, we might open
the WAL at a snapshot position that was never saved to the WAL.
See etcd-io/etcd#8082, etcd-io/etcd#8088.