kv: use response keys of ranged writes for lock tracking#121086
kv: use response keys of ranged writes for lock tracking#121086craig[bot] merged 4 commits intocockroachdb:masterfrom
Conversation
|
This change is |
c0fff40 to
1136532
Compare
This is an old test which is tightly coupled with how locks are tracked on the client. In particular, it expects all ranged locking requests to use `ResolveIntentRange` requests to resolve their locks; this is expected to change immminently (see cockroachdb#121086). It's also cumbersome to change given the randomization it uses to construct batches. We've got coverage for things being tested here elsewhere. There's some integration tests in `intent_resolver_integration_test.go` that test things deterministically. There's also tests for the txn pipeliner and txn committer that test lock tracking on the client. I'm probably missing some other examples as well, but deleting this should be safe. Epic: none Release note: None
arulajmani
left a comment
There was a problem hiding this comment.
This looks good. Let me know when the tests are updated, and I'll give this one more look and stamp it!
Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
-- commits line 20 at r3:
Nice!
-- commits line 39 at r4:
#121119 should help. I found it very cumbersome to update this test and still keep it meaningful while working on the DeleteRange pipelining PR yesterday, so I punted the problem to this PR (sorry!)
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 758 at r4 (raw file):
log.Fatalf(ctx, "unexpected multi-key intent pipelined") } tp.ifWrites.insert(span.Key, seq)
nit: mind adding a TODO somewhere to rename this to ifLocks or something similar?
pkg/kv/kvclient/kvcoord/txn_coord_sender_test.go line 1107 at r4 (raw file):
defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) skip.IgnoreLint(t, "WIP")
TIL this is a thing!
120861: sql/pg_catalog: fix attname for dropped columns r=rafiss a=Jeremyyang920 Previously, the attname columns for dropped columns was padded used a %8d string format. This led to incorrect behavior as it meant a total of 8 '.' characters inclusive of the column ordinal as compared to the PG code base which always pads with 8 '.'. This affected migration tooling since it attempts to verify column names using the returned attname and led to mismatching column reports. This commit fixes the issue by explicitly setting the padding to 8 '.' to match the PG implementation. Fixes: #120855 Release note (bug fix): attname for dropped columns is now properly padded with 8 '.' to match PG. 120941: roachprod: add faster restarts, prettier multi-node sql printing r=dt a=dt A couple minor quality-of-life improvements motivated by working on long-lived, multi-node DRT clusters. 121119: server: delete TestIntentResolution r=arulajmani a=arulajmani This is an old test which is tightly coupled with how locks are tracked on the client. In particular, it expects all ranged locking requests to use `ResolveIntentRange` requests to resolve their locks; this is expected to change immminently (see #121086). It's also cumbersome to change given the randomization it uses to construct batches. We've got coverage for things being tested here elsewhere. There's some integration tests in `intent_resolver_integration_test.go` that test things deterministically. There's also tests for the txn pipeliner and txn committer that test lock tracking on the client. I'm probably missing some other examples as well, but deleting this should be safe. Epic: none Release note: None Co-authored-by: Jeremy Yang <jyang@cockroachlabs.com> Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
This commit adds a unit test for ResponseKeyIterate. Epic: None Release note: None
This commit updates ResponseKeyIterate to reject a nonsensical case. Epic: None Release note: None
This commit updates ResponseKeyIterate to handle DeleteRange requests that have their ReturnKeys flag set to true. Epic: None Release note: None
1136532 to
dee7418
Compare
Informs cockroachdb#117978. This commit updates the `txnPipeliner` to use the response keys from `Get`, `Scan`, `ReverseScan`, and `DeleteRange` requests to track pipelined and non-pipelined lock acquisitions / intent writes, instead of assuming that the requests could have left intents anywhere in their request span. This more precise tracking avoid broad ranged intent resolution when more narrow intent resolution is possible. It will also be used by cockroachdb#117978 to track in-flight replicated lock acquisition. Release note: None
dee7418 to
10c4268
Compare
nvb
left a comment
There was a problem hiding this comment.
TFTR! I've addressed the comments and updated all tests. This should be good for another look.
Reviewable status:
Previously, arulajmani (Arul Ajmani) wrote…
#121119 should help. I found it very cumbersome to update this test and still keep it meaningful while working on the DeleteRange pipelining PR yesterday, so I punted the problem to this PR (sorry!)
Thanks! This was failing before the rebase.
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 758 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: mind adding a TODO somewhere to rename this to
ifLocksor something similar?
Done.
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 7 of 7 files at r5, 3 of 3 files at r6, 2 of 2 files at r7, 3 of 6 files at r8.
Reviewable status:
|
TFTR! bors r+ |
121088: kv: pipeline replicated lock acquisition r=nvanbenschoten a=nvanbenschoten Fixes #117978. Builds upon the foundation laid in [#119975](#119975), [#119933](#119933), [#121065](#121065), and [#121086](#121086). This commit completes the client-side handling of replicated lock acquisition pipelining. Replicated lock acquisition through `Get`, `Scan`, and `ReverseScan` requests now qualifies to be pipelined. The `txnPipeliner` is updated to track the strength associated with each in-flight write and pass that along to the corresponding `QueryIntentRequest`. See [benchmark with TPC-C results here](#121088 (comment)). Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Informs #117978.
This PR updates the
txnPipelinerto use the response keys fromGet,Scan,ReverseScan, andDeleteRangerequests to track pipelined and non-pipelined lock acquisitions / intent writes, instead of assuming that the requests could have left intents anywhere in their request span. This more precise tracking avoid broad ranged intent resolution when more narrow intent resolution is possible. It will also be used by #117978 to track in-flight replicated lock acquisition.This is a WIP. Some tests need to be updated.
Release note: None