-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Move checkpoint key to WAL #3444
Conversation
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.
Reviewed 4 of 6 files at r1, 1 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain)
worker/draft.go, line 716 at r2 (raw file):
if first, err := n.Store.FirstIndex(); err == nil { // Save some cycles by only calculating snapshot if the checkpoint has gone // quite a bit further than first index.
nit: than the first index
worker/draft.go, line 1172 at r2 (raw file):
// At i7, min pending start ts = S3, therefore snapshotIdx = i5 - 1 = i4. // At i7, max commit ts = C1, therefore readTs = C1. func (n *node) calculateSnapshot(startIdx uint64, discardN int) (*pb.Snapshot, error) {
Maybe edit the doc comment to include something about why startIdx is now being passed as an argument.
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.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @golangcibot and @martinmr)
worker/draft.go, line 733 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
Error return value of
x.Errorf
is not checked (fromerrcheck
)
Done.
worker/draft.go, line 1172 at r2 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Maybe edit the doc comment to include something about why startIdx is now being passed as an argument.
Done.
The checkpoint key was being stored in p directory. Can't think of a reason why I did that. Keeping the checkpoint key in WAL makes natural sense and allows someone to drop the w directory, without causing issues later. One of the issues is that if w is dropped, Raft index would start from 1 again, but p directory would have stored the progress to a much higher index. This would cause all new proposals to not be applied. With this PR, w directory can be dropped and the checkpoint would also get dropped along with it. This PR also adds two optimizations: Calculates checkpoints starting from the last checkpoint, instead of the first entry since snapshot. Does not calculate snapshot if the checkpoint - first < SnapshotAfter.
The checkpoint key was being stored in p directory. Can't think of a reason why I did that. Keeping the checkpoint key in WAL makes natural sense and allows someone to drop the w directory, without causing issues later. One of the issues is that if w is dropped, Raft index would start from 1 again, but p directory would have stored the progress to a much higher index. This would cause all new proposals to not be applied. With this PR, w directory can be dropped and the checkpoint would also get dropped along with it. This PR also adds two optimizations: Calculates checkpoints starting from the last checkpoint, instead of the first entry since snapshot. Does not calculate snapshot if the checkpoint - first < SnapshotAfter.
The checkpoint key was being stored in p directory. Can't think of a reason why I did that. Keeping the checkpoint key in WAL makes natural sense and allows someone to drop the w directory, without causing issues later. One of the issues is that if w is dropped, Raft index would start from 1 again, but p directory would have stored the progress to a much higher index. This would cause all new proposals to not be applied.
With this PR, w directory can be dropped and the checkpoint would also get dropped along with it.
This PR also adds two optimizations:
This change is