Skip to content

kv: support locking Get requests#59023

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
nvb:nvanbenschoten/mergeLock
Jan 31, 2021
Merged

kv: support locking Get requests#59023
craig[bot] merged 2 commits intocockroachdb:masterfrom
nvb:nvanbenschoten/mergeLock

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Jan 14, 2021

This commit adds support for Get requests to acquire unreplicated locks in cases where they find a present key-value. This is supported by a new field on GetRequest called KeyLocking, which mirrors the field of the same name found on ScanRequest and ReverseScanRequest.

Much of this commit was modeled after a98be1f, which was the original commit that added locking support to ScanRequest and ReverseScanRequest.

The intention is to use this functionality to fix #46639.

@nvb nvb requested a review from andreimatei January 14, 2021 23:38
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@nvb nvb force-pushed the nvanbenschoten/mergeLock branch 2 times, most recently from e6c4349 to 5d97dd6 Compare January 15, 2021 00:55
@nvb nvb requested review from aayushshah15 and removed request for andreimatei January 26, 2021 17:58
nvb added 2 commits January 26, 2021 19:59
This commit adds support for Get requests to acquire unreplicated locks
in cases where they find a present key-value. This is supported by a new
field on GetRequest called KeyLocking, which mirrors the field of the
same name found on ScanRequest and ReverseScanRequest.

Much of this commit was modeled after a98be1f, which was the original
commit that added locking support to ScanRequest and ReverseScanRequest.

The intention is to use this functionality to fix cockroachdb#46639.
This commit extends kvnemesis to issue locking GetRequests. Much of this
is modeled after a98be1f, which added support to kvnemesis for locking
ScanRequests.
@nvb nvb force-pushed the nvanbenschoten/mergeLock branch from 5d97dd6 to c955e88 Compare January 27, 2021 01:00
Copy link
Copy Markdown
Contributor

@aayushshah15 aayushshah15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pointer to your SELECT FOR UPDATE PR was very helpful.

:lgtm: but most of my review on it was me understanding it rather than scrutinizing it. I assume that that's okay since it's only reusing the machinery introduced in that above PR.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/kv/db_test.go, line 90 at r1 (raw file):

	defer s.Stopper().Stop(context.Background())

	result, err := db.GetForUpdate(context.Background(), "aa")

Do you know why these two Get tests are written the way they are? It seems like they're intentionally "simplistic".


pkg/roachpb/api.go, line 1170 at r1 (raw file):

func (gr *GetRequest) flags() int {
	maybeLocking := flagForLockStrength(gr.KeyLocking)

For my understanding. why the maybe here? Is it because the unreplicated lock is only acquired if the key exists?

Copy link
Copy Markdown
Contributor Author

@nvb nvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

bors r+

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15)


pkg/kv/db_test.go, line 90 at r1 (raw file):

Previously, aayushshah15 (Aayush Shah) wrote…

Do you know why these two Get tests are written the way they are? It seems like they're intentionally "simplistic".

They seem to be quite old tests. In fact, they were originally Example-style tests, and were converted to normal unit tests in 12f8d1d in response to #6101.


pkg/roachpb/api.go, line 1170 at r1 (raw file):

Previously, aayushshah15 (Aayush Shah) wrote…

For my understanding. why the maybe here? Is it because the unreplicated lock is only acquired if the key exists?

I think it's actually because if KeyLocking is set to None then this flag is not set. So maybe is indicating the flag may or may or may not be non-zero.

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 31, 2021

Build succeeded:

@craig craig bot merged commit 6b58038 into cockroachdb:master Jan 31, 2021
@nvb nvb deleted the nvanbenschoten/mergeLock branch February 1, 2021 02:25
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.

kv: kvnemesis can thrash and livelock on multiple concurrent Range merges

3 participants