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

etcdserver: purge old snap.db files #7967

Merged
merged 1 commit into from
May 30, 2017

Conversation

heyitsanthony
Copy link
Contributor

Lots of garbage db files in #7957. Should purge.

Lots of garbage db files in etcd-io#7957. Should purge.
Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm. thanks!

@xiang90
Copy link
Contributor

xiang90 commented May 23, 2017

lgtm

@fanminshi
Copy link
Member

@heyitsanthony can this potentially affect recovering db path? suppose there are 5 .snap files already and 1 xxx.snap.db from leader (maxsnap = 5). while etcdserver is applying snapshot and purgefile go routine deletes xxx.snap.db. then xxx.snap.db can't be found and etcdserver panic.

@heyitsanthony
Copy link
Contributor Author

@fanminshi if that's the case, why is it safe to purge the v2 .snap files? Can you reproduce this issue where the server needs a backlog of five snapshots?

@fanminshi
Copy link
Member

@heyitsanthony it seems to me that it is possible that an etcd can potentially have more than one .snap files and one .snap.db file. That's why we purge then if greater than some threshold right? In the case of #7957, there are already more than N xxx.snap.db files. If the example I gave just have 6 xxx.snap.db files, where the 6th one is from from the leader. while follower is applying the 6th xxx.snap.db file and the snap.db file gets deleted by purge function. will that be a issue then?

@heyitsanthony
Copy link
Contributor Author

@fanminshi the purge is sorted; etcd will only purge the oldest files. I'm asking if .snap purging is safe, then why wouldn't .snap.db purging also be safe? If .snap purging is not safe, there needs to be a test case around it that breaks using the current purging policy.

@fanminshi
Copy link
Member

@heyitsanthony I see. if purge only deletes oldest one, then purge is safe and my concern doesn't matter. LGTM

@heyitsanthony heyitsanthony merged commit a20e667 into etcd-io:master May 30, 2017
@heyitsanthony heyitsanthony deleted the purge-snapdb branch May 30, 2017 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants