Skip to content

storage: fix overaggressive raft log truncation#8636

Merged
petermattis merged 1 commit intocockroachdb:masterfrom
petermattis:pmattis/raft-log-queue
Aug 18, 2016
Merged

storage: fix overaggressive raft log truncation#8636
petermattis merged 1 commit intocockroachdb:masterfrom
petermattis:pmattis/raft-log-queue

Conversation

@petermattis
Copy link
Copy Markdown
Collaborator

@petermattis petermattis commented Aug 18, 2016 •
edited
Loading

Fixes #8629.

This change is Reviewable

spencerkimball
spencerkimball reviewed Aug 18, 2016
View reviewed changes
storage/raft_log_queue.go Outdated
truncatableIndex := raftStatus.Commit
behindIndexes := getBehindIndexes(raftStatus)
if raftLogSize <= targetSize {
if raftLogSize <= targetSize && len(behindIndexes) > 0 {
Copy link
Copy Markdown
Member

@spencerkimball spencerkimball Aug 18, 2016

Choose a reason for hiding this comment

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

By the way, why in the heck are we computing a unique array of behind indexes in getBehindIndexes? That's just silly. Could you remove that extra complication?

Copy link
Copy Markdown
Collaborator Author

@petermattis petermattis Aug 18, 2016

Choose a reason for hiding this comment

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

Done.

@spencerkimball
Copy link
Copy Markdown
Member

spencerkimball commented Aug 18, 2016

LGTM

@petermattis petermattis force-pushed the pmattis/raft-log-queue branch from 14bf1a5 to e74baa8 Compare August 18, 2016 13:34
@cuongdo
Copy link
Copy Markdown
Contributor

cuongdo commented Aug 18, 2016

:lgtm:

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

storage/raft_log_queue.go, line 135 [r1] (raw file):

Previously, spencerkimball (Spencer Kimball) wrote…

By the way, why in the heck are we computing a unique array of behind indexes in getBehindIndexes? That's just silly. Could you remove that extra complication?

+1 the uniqueness isn't needed, but the sorting is

Comments from Reviewable

@cuongdo
Copy link
Copy Markdown
Contributor

cuongdo commented Aug 18, 2016

storage/raft_log_queue.go, line 156 [r2] (raw file):

  for _, progress := range raftStatus.Progress {
      index := progress.Match
      if index < raftStatus.Commit && behind > index {

is index < raftStatus.Commit needed? behind starts at raftStatus.Commit and can only decrease from there because of the behind > index condition

Comments from Reviewable

@cuongdo
Copy link
Copy Markdown
Contributor

cuongdo commented Aug 18, 2016

storage/raft_log_queue.go, line 156 [r2] (raw file):

Previously, cuongdo (Cuong Do) wrote…

is index < raftStatus.Commit needed? behind starts at raftStatus.Commit and can only decrease from there because of the behind > index condition

also, `behind > index` would read more naturally to me as `index < behind`, but this is a nit, so your call

Comments from Reviewable

@petermattis
Copy link
Copy Markdown
Collaborator Author

petermattis commented Aug 18, 2016

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

storage/raft_log_queue.go, line 156 [r2] (raw file):

Previously, cuongdo (Cuong Do) wrote…

also, behind > index would read more naturally to me as index < behind, but this is a nit, so your call

What if `progress.Match > raftStatus.Commit`? I'd rather be conservative here.

Comments from Reviewable

@cuongdo
Copy link
Copy Markdown
Contributor

cuongdo commented Aug 18, 2016

storage/raft_log_queue.go, line 156 [r2] (raw file):

Previously, petermattis (Peter Mattis) wrote…

What if progress.Match > raftStatus.Commit? I'd rather be conservative here.

suppose: 
progress.Match = 2
raftStatus.Commit = 1

behind would start at 1. behind (1) isn't greater than progress.Match (2), so `behind isn't updated. However, being conservative with this code is probably a good thing.

Comments from Reviewable

@cuongdo
Copy link
Copy Markdown
Contributor

cuongdo commented Aug 18, 2016

still :lgtm:

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

@petermattis petermattis force-pushed the pmattis/raft-log-queue branch from e74baa8 to 70f7a56 Compare August 18, 2016 13:53
@petermattis petermattis mentioned this pull request Aug 18, 2016
Closed
@petermattis petermattis merged commit 4172cbc into cockroachdb:master Aug 18, 2016
@petermattis petermattis deleted the pmattis/raft-log-queue branch August 18, 2016 14:43
@bdarnell bdarnell mentioned this pull request Aug 18, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@bdarnell bdarnell

@cuongdo cuongdo

@spencerkimball spencerkimball

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@petermattis @spencerkimball @cuongdo @bdarnell