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: prevent quiescence if Raft leadership transfer is in progress #18202

Merged

Conversation

petermattis
Copy link
Collaborator

Add TestReplicaShouldQuiesce to test both the new functionality and the
existing conditions which prevent quiescence.

See #13523

@petermattis petermattis requested a review from a team September 4, 2017 20:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis petermattis requested a review from bdarnell September 4, 2017 20:21
@petermattis petermattis force-pushed the pmattis/quiesce-leader-transfer branch 2 times, most recently from e4ab255 to 719ea48 Compare September 5, 2017 01:37
@tbg
Copy link
Member

tbg commented Sep 5, 2017

:lgtm:, though @bdarnell should take a look as well.


Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/storage/replica.go, line 3749 at r1 (raw file):

	if status.Commit != lastIndex {
		if log.V(4) {
			log.Infof(ctx, "not quiescing: commit (%d) != last-index (%d)",

nit while you're here: s/last-index/lastIndex/


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

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


pkg/storage/replica.go, line 3749 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

nit while you're here: s/last-index/lastIndex/

Done.


Comments from Reviewable

@bdarnell
Copy link
Contributor

bdarnell commented Sep 5, 2017

:lgtm:


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


pkg/storage/replica.go, line 3661 at r2 (raw file):

type quiescer interface {
	Clock() *hlc.Clock

Maybe shouldReplicaQuiesce should take a now hlc.Timestamp instead of having access to a Clock.


Comments from Reviewable

Split maybeQuiesceLocked into shouldReplicaQuiesce and
quiesceAndNotifyLocked. shouldReplicaQuiesce is structured to take an
interface to allow for testing. Add test cases for all of the conditions
that cause shouldReplicaQuiesce to return false.

Fixes cockroachdb#18183
@petermattis petermattis force-pushed the pmattis/quiesce-leader-transfer branch from c22acaf to b4949cc Compare September 5, 2017 15:43
@petermattis
Copy link
Collaborator Author

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


pkg/storage/replica.go, line 3661 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Maybe shouldReplicaQuiesce should take a now hlc.Timestamp instead of having access to a Clock.

I was anxious about there being too many arguments to that method. Let me see what that looks like.

Ok. Done.


Comments from Reviewable

@petermattis petermattis merged commit a1e3a7f into cockroachdb:master Sep 5, 2017
@petermattis petermattis deleted the pmattis/quiesce-leader-transfer branch September 5, 2017 16:21
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.

4 participants