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

storage: eagerly replicate after splits only if not replicated #7396

Merged

Conversation

petermattis
Copy link
Collaborator

@petermattis petermattis commented Jun 22, 2016


This change is Reviewable

@petermattis
Copy link
Collaborator Author

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


storage/replica_command.go, line 2317 [r2] (raw file):

          r.store.replicateQueue.MaybeAdd(r, r.store.Clock().Now())
      }
      if len(split.NewDesc.Replicas) == 1 {

This is the key line which avoids replicating if we're also campaigning. Totally unclear to me why replicating while campaigning would be problematic. And note that the replica scanner could decide to replicate anyways so the bug seen in #7386 still exists without this PR, it is just much more rare.

@tschottdorf, @bdarnell I need your guidance here.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

PS I'm in no hurry to merge this.


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


Comments from Reviewable

@tbg
Copy link
Member

tbg commented Jun 22, 2016

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


storage/replica_command.go, line 2317 [r2] (raw file):

Previously, petermattis (Peter Mattis) wrote…

This is the key line which avoids replicating if we're also campaigning. Totally unclear to me why replicating while campaigning would be problematic. And note that the replica scanner could decide to replicate anyways so the bug seen in #7386 still exists without this PR, it is just much more rare.

@tschottdorf, @bdarnell I need your guidance here.

I'm looking.

Comments from Reviewable

@petermattis petermattis force-pushed the pmattis/debug-eager-replication branch from b145b51 to a0f86dd Compare June 23, 2016 12:01
@petermattis
Copy link
Collaborator Author

This is ready for another look. The first commit is revert of a revert.


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


storage/replica_command.go, line 2314 [r4] (raw file):

      // both new halves to speed up a potentially necessary replication. See
      // #7022.
      if len(split.UpdatedDesc.Replicas) == 1 {

I've decided to leave these restrictions in for now. The address @BramGrunier's concern that we're calling replicaQueue.MaybeAdd unnecessarily while still achieve fast replication of the initial ranges.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Jun 23, 2016

:lgtm: except for the "Fixes #7386" bit, since that underlying bug is still present.


Reviewed 6 of 6 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

Yeah, I'll remove that. #7386 was fixed by #7404.


Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@petermattis petermattis force-pushed the pmattis/debug-eager-replication branch from a0f86dd to fa6a986 Compare June 23, 2016 13:43
@petermattis petermattis merged commit 4839efe into cockroachdb:master Jun 23, 2016
@petermattis petermattis deleted the pmattis/debug-eager-replication branch June 23, 2016 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants