Skip to content
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

Merged
merged 4 commits into from
Jun 14, 2017
Merged

net/raft: save snapshot position first #1312

merged 4 commits into from
Jun 14, 2017

Conversation

jbowens
Copy link
Contributor

@jbowens jbowens commented Jun 12, 2017

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.

@jbowens
Copy link
Contributor Author

jbowens commented Jun 13, 2017

PTAL

CoreOS confirmed this in etcd-io/etcd#8088.

Copy link
Contributor

@tessr tessr left a 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)
Copy link
Contributor

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.
@iampogo iampogo merged commit cbde280 into main Jun 14, 2017
@iampogo iampogo deleted the wal-snap-first branch June 14, 2017 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants