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

database/raft: save snapshot position first #1310

Merged
merged 3 commits into from
Jun 13, 2017
Merged

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.

This is a fix for the 1.2.x release line.

See etcd-io/etcd#8082, etcd-io/etcd#8088.

@jbowens
Copy link
Contributor Author

jbowens commented Jun 13, 2017

PTAL. This is the 1.2.x version of #1312.

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 - I'd make the same comment as about #1312 (should we refactor these two things into a single function) but for a 1.2 patch we should probably minimize refactoring, 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. Also, it's debatable whether this meets the
requirements for a point release given that this bug has a very low
probablity of surfacing. WDYT?
@iampogo iampogo merged commit f29c712 into 1.2-stable Jun 13, 2017
@iampogo iampogo deleted the save-snapshot branch June 13, 2017 21:27
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