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: fix quiescence when not leaseholder #18182

Conversation

petermattis
Copy link
Collaborator

Fix check for whether we own a valid lease.

Fixes #17741

@petermattis petermattis requested a review from a team September 2, 2017 17:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis petermattis requested a review from bdarnell September 2, 2017 17:26
@petermattis petermattis force-pushed the pmattis/do-not-quiesce-not-leaseholder branch 2 times, most recently from efe92e4 to 3418678 Compare September 2, 2017 17:34
Fix check for whether we own a valid lease.

Fixes cockroachdb#17741
@petermattis petermattis force-pushed the pmattis/do-not-quiesce-not-leaseholder branch from 3418678 to 06486d4 Compare September 2, 2017 17:48
@bdarnell
Copy link
Contributor

bdarnell commented Sep 2, 2017

:lgtm:

We should go ahead and merge this (and assuming it fixes the problem, patch it in for the beta), but we need a follow-up issue to add more testing for quiescence and specifically a test that would be sensitive to this issue.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

Merging and filing an issue to test this area of quiescence. I won't be able to cherry-pick into the release branch until later tonight (after 8pm).

@petermattis petermattis merged commit 6cc0f3e into cockroachdb:master Sep 2, 2017
@petermattis petermattis deleted the pmattis/do-not-quiesce-not-leaseholder branch September 2, 2017 20:07
@bdarnell
Copy link
Contributor

bdarnell commented Sep 2, 2017

OK, I'll do the cherry-pick in #18184

@bdarnell
Copy link
Contributor

bdarnell commented Sep 2, 2017

adriatic had been in the zero qps state and recovered when this change was deployed (not conclusive since it doesn't always get stuck, but a good sign). omega and cerulean were fine before this change was deployed and fine after. omega appeared to suffer more disruption to traffic during the push than usual.

@petermattis
Copy link
Collaborator Author

petermattis commented Sep 3, 2017

@bdarnell adriatic and omega are both running 8633ca4 which does not have your cherry-pick (3cab35b).

@bdarnell
Copy link
Contributor

bdarnell commented Sep 3, 2017

adriatic and omega are both running 8633ca4 which does have your cherry-pick (3cab35b).

That's what I mean. I was describing the push of 8633ca4 to the test clusters.

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