Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upstorage: avoid merge deadlock when concurrent split wins #31294
Conversation
benesch
requested review from
bdarnell and
tschottdorf
Oct 12, 2018
benesch
requested a review
from cockroachdb/core-prs
as a
code owner
Oct 12, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
bdarnell
approved these changes
Oct 12, 2018
That's a surprisingly simple test for such a complex issue.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
pkg/storage/client_merge_test.go, line 1133 at r1 (raw file):
if bt := req.GetBeginTransaction(); bt != nil && bt.Key.Equal(keys.RangeDescriptorKey(roachpb.RKeyMin)) && atomic.AddInt64(&launchSplit, -1) == 0 {
Nit: Why use a negative value here?
tschottdorf
approved these changes
Oct 12, 2018
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (and 1 stale)
pkg/storage/client_merge_test.go, line 1133 at r1 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
Nit: Why use a negative value here?
I'm confused by the comment, Ben. He could use a CAS, is that what you mean? Or subtract 1 below and add +1 here?
bdarnell
reviewed
Oct 12, 2018
Reviewable status:
complete! 2 of 0 LGTMs obtained
pkg/storage/client_merge_test.go, line 1133 at r1 (raw file):
Previously, tschottdorf (Tobias Schottdorf) wrote…
I'm confused by the comment, Ben. He could use a CAS, is that what you mean? Or subtract 1 below and add +1 here?
I missed the Store below and was just confused. Nevermind (although I think a CAS might capture the intent a little more cleanly).
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
benesch
Oct 15, 2018
Member
Ack, switched to a CAS. Thanks for the reviews!
bors r=tschottdorf,bdarnell
|
Ack, switched to a CAS. Thanks for the reviews! bors r=tschottdorf,bdarnell |
bot
pushed a commit
that referenced
this pull request
Oct 15, 2018
benesch
referenced this pull request
Oct 15, 2018
Closed
roachtest: clearrange/checks=false failed #31201
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
craig
bot
commented
Oct 15, 2018
Build succeeded |
benesch commentedOct 12, 2018
@bdarnell, in a stroke of luck, the existing conditional put API supports deleting the existing value when you pass nil.
When merging two adjacent ranges P and Q, we need to be careful to
delete Q's range descriptor with a conditional put, or we can deadlock
with a concurrent split. See the comments and test case within for
details. This commit fixes the last issue that is blocking merges from
being turned on by default.
Release note: None