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

Raft: do not ever remove state, rename it and leave it around #233

Merged
merged 2 commits into from Nov 14, 2017

Conversation

hsanjuan
Copy link
Collaborator

This commit changes the way that consensus.Clean() works. Before
it deleted the whole data folder. Now it renames it as .old.0
and leaves it. When Clean() is called again, it renames .old.0
as .old.1, and the actual data becomes .old.0. Higher number
means older. The number of backups is fixed to 5. When 5 backups exists
and a new one comes up again, the last one is discarded.

License: MIT
Signed-off-by: Hector Sanjuan hector@protocol.ai

This commit changes the way that consensus.Clean() works. Before
it deleted the whole data folder. Now it renames it as <name>.old.0
and leaves it. When Clean() is called again, it renames <name>.old.0
as <name>.old.1, and the actual data becomes <name>.old.0. Higher number
means older. The number of backups is fixed to 5. When 5 backups exists
and a new one comes up again, the last one is discarded.

License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@ghost ghost assigned hsanjuan Nov 13, 2017
@ghost ghost added the status/in-progress In progress label Nov 13, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 74.8% when pulling 1a06bae on feat/raft-state-backups into 435012a on master.

@hsanjuan hsanjuan added need/review Needs a review and removed status/in-progress In progress labels Nov 14, 2017
@hsanjuan hsanjuan mentioned this pull request Nov 14, 2017
Copy link
Collaborator

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small thing about error messages

}
backups := helper.listBackups()
if i < RaftDataBackupKeep && len(backups) != i+1 {
t.Fatal("not saving enough backups")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If something really weird happened with the helper and too many backups were saved before iteration RaftDataBackupKeep or alternatively too few backups were saved after iteration RaftDataBackupKeep, then the error messages will be misleading. As the check stands, a better message would be "incorrect number of backups saved"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok fixed

License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@ghost ghost added the status/in-progress In progress label Nov 14, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 74.687% when pulling 2837170 on feat/raft-state-backups into 435012a on master.

@ZenGround0
Copy link
Collaborator

LGTM

@hsanjuan hsanjuan merged commit 4c3f16d into master Nov 14, 2017
@ghost ghost removed the status/in-progress In progress label Nov 14, 2017
@hsanjuan hsanjuan deleted the feat/raft-state-backups branch November 14, 2017 17:13
@hsanjuan
Copy link
Collaborator Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants