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

wip: kv: evict leaseholder on RPC error #23938

Closed
wants to merge 1 commit into from

Conversation

tbg
Copy link
Member

@tbg tbg commented Mar 16, 2018

This addresses a situation in which we would not evict a stale
leaseholder for a long time. Consider the replica [s1,s2,s3] and
s1 is down but is the cached leaseholder, while s2 is the actual lease
holder. The RPC layer will try s1, get an RPC error, try s2 and succeed.
Since there is no NotLeaseHolderError involved, the cache would not get
updated, and so every request pays the overhead of trying s1 first.

WIP because needs testing.

Touches #23601.

Release note (bug fix): Improve request routing during node outages.

Which is what previously happened, thanks to the call to t.Fatal
which in turn called runtime.Goexit. We need to call `done` here
so that the caller has their WaitGroup signaled.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member Author

tbg commented Mar 16, 2018

Typed faster than my eyes could follow.

@tbg tbg closed this Mar 16, 2018
@tbg tbg deleted the roachtest/panic branch March 16, 2018 01:07
@tbg tbg restored the roachtest/panic branch March 16, 2018 01:07
@tbg tbg deleted the roachtest/panic branch March 16, 2018 01:33
@tbg tbg restored the roachtest/panic branch April 16, 2018 15:27
@tbg tbg deleted the roachtest/panic branch May 9, 2018 01:34
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.

2 participants