Conversation
martinmr
left a comment
There was a problem hiding this comment.
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.
manishrjain
left a comment
There was a problem hiding this comment.
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.Errorfis 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