kv: prep in-flight write tracking for replicated locks#121065
kv: prep in-flight write tracking for replicated locks#121065craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
This change is |
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
-- commits line 16 at r3:
Should we add a test for this, or modify an existing test?
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 963 at r3 (raw file):
if delItem != nil { // An in-flight write with the same key and sequence already existed in the // set. This is unexpected, but we handle it.
Is this worth a warning?
arulajmani
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 963 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Is this worth a warning?
(nit)
nvb
left a comment
There was a problem hiding this comment.
TFTRs!
Reviewable status:
Previously, arulajmani (Arul Ajmani) wrote…
Should we add a test for this, or modify an existing test?
There are tests for this. For example, if I comment out w.chainIndex = chainIndex, TestTxnPipelinerRangedWrites fails.
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 963 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
(nit)
Eh, I don't think there needs to be. inFlightWriteSet shouldn't really care, as long as it handles the situation correctly. It may also be possible if we acquire two replicated locks at the same seq num, because we don't increment the seq num across locking reads. I'll be changing this comment to mention that when we add replicated lock pipelining.
arulajmani
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
There are tests for this. For example, if I comment out
w.chainIndex = chainIndex,TestTxnPipelinerRangedWritesfails.
Ack, as long as there's something!
pkg/kv/kvclient/kvcoord/txn_interceptor_pipeliner.go line 963 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Eh, I don't think there needs to be.
inFlightWriteSetshouldn't really care, as long as it handles the situation correctly. It may also be possible if we acquire two replicated locks at the same seq num, because we don't increment the seq num across locking reads. I'll be changing this comment to mention that when we add replicated lock pipelining.
Makes sense!
Informs cockroachdb#94731. This probably isn't working correctly. Make a note of it in the code. Release note: None
Informs cockroachdb#117978. This commit simplifies the logic in txnPipeliner.chainToInFlightWrites which ensures that we only add a single QueryIntent request to the BatchRequest per overlapping in-flight write. In doing so, it eliminates an unnecessary hash-map by piggy-backing tracking onto the existing btree. It also avoids making an assumption that we only track a single in-flight write per key. Release note: None
Informs cockroachdb#117978. This commit simplifies the logic in `inFlightWriteSet` to track in-flight writes with the same key but different sequence numbers separately. This simplification is done to avoid confusion around in-flight writes with different seq nums and/or different strengths (which will be added shortly), and whether any of these in-flight writes should imply that the others are no longer in-flight. Release note: None
629e830 to
2aa3762
Compare
|
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 includes a pair of simplifications to the
txnPipelinerto make fewer assumptions about in-flight writes, as a way to prepare for #117978.kv: remove chainedKeys hash-map from chainToInFlightWrites, simplify
This commit simplifies the logic in txnPipeliner.chainToInFlightWrites which ensures that we only add a single QueryIntent request to the BatchRequest per overlapping in-flight write. In doing so, it eliminates an unnecessary hash-map by piggy-backing tracking onto the existing btree. It also avoids making an assumption that we only track a single in-flight write per key.
kv: track in-flight writes with same key and different seq nums
This commit simplifies the logic in
inFlightWriteSetto track in-flight writes with the same key but different sequence numbers separately. This simplification is done to avoid confusion around in-flight writes with different seq nums and/or different strengths (which will be added shortly), and whether any of these in-flight writes should imply that the others are no longer in-flight.Release note: None